Handle partial-success responses for OTLP trace #3106
Conversation
jmacd
requested review from
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3106 +/- ##
=====================================
Coverage 76.4% 76.4%
=====================================
Files 179 180 +1
Lines 11977 12014 +37
=====================================
+ Hits 9160 9190 +30
- Misses 2578 2583 +5
- Partials 239 241 +2
|
Co-authored-by: David Ashpole <dashpole@google.com>
Co-authored-by: David Ashpole <dashpole@google.com>
…o jmacd/partial_success
hanyuancheung
force-pushed
the
jmacd/partial_success
branch
from
09bd516
to
8723d2e
Compare
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
|
Please consider 54d9dcf a question about readability. I rewrote the code to have better coverage (to pass the CI checks), but I'm not sure it's a good trade. OTOH, I feel it's not worth adding specialized tests to ensure that errors-in-response-handling (protobuf error, I/O error, etc.) are covered. What do you think? |
|
I also prefer the lower-code-coverage version for readability. I would support merging even if it does not pass those checks. Was it failing the |
This reverts commit 54d9dcf. Mixed results from the style question. We prefer less test, more readability it seems.
|
I reverted the otlpmetric portion. I also reverted the questionable style in 54d9dcf. (This is no longer a priority for me, however 😁) |
jmacd
changed the title
jmacd
changed the title
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package otlp // import "go.opentelemetry.io/otel/exporters/otlp" |
There was a problem hiding this comment.
I'm conflicted about where this should live. On the one hand, I would like users to create an ErrorHandler that does something more intelligent than just log a string. On the other hand, I would prefer not to expose a new API surface needlessly.
If no one has any other objections to this than I can live with it.
There was a problem hiding this comment.
I'm a fan of structured logging; I would advocate for a backwards-compatible mechanism to accomplish it. The PartialSuccess error can implement some future API that enables it to be expressed in a structured way when the producer logs it to a structure-aware logger.
I didn't know where to put it either. At this level in the current package structure, it falls into the top-level package, which feels OK to me.
There was a problem hiding this comment.
With no objections to this, we can leave it where it is.
There was a problem hiding this comment.
I would rather have these live in an internal package, at lease to start.
There was a problem hiding this comment.
I'm not sure what a user will be able to do with an error handler other than log the error given they cannot retry the export. And adding this to the public API seems like adding something only OTel developers will use or care about, not users.
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
…y-go into jmacd/partial_success
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
|
There was an unresolved comment: #3106 (comment)
I think this PR was merged pre-maturely. |
I'm opening a to address this. |
Fixes #3102.
This constructs an
errorfor passing tootel.Handle()from any of the new PartialSuccess messages in OTLP v0.19.See the specification of this feature.