Emit outcome: failure in obsconsumer

[jade-guiton-dd]

Description

The last remaining part of #12676 is to implement the outcome: failure part of the Pipeline Component Telemetry RFC (see here). This is done by introducing a downstream error wrapper struct, to distinguish between errors coming from the next component from errors bubbled from further downstream.

Important note

This PR implements things slightly differently from what the text of the RFC describes.

If a pipeline contains components A → B and an error occurs in B, this PR records:

• otelcol.component.outcome = failure in the otelcol.*.consumed.* metric for B
• otelcol.component.outcome = refused in the otelcol.*.produced.* metric for A

whereas the RFC would set both outcomes to failure.

This is programmatically simpler — no need to have different behavior between the obsconsumer around the output of A and the one around the input of B — but more importantly, I think it is clearer for users as well: outcome = failure only occurs on metrics associated with the component where the failure actually occurred.

This subtlety wasn't discussed in-depth in #11956 which introduced outcome = refused, so I took the liberty to make this change. If necessary, I can file another RFC amendment to match, or, if there are objections, implement the RFC as-is instead.

Link to tracking issue

Fixes #12676

Testing

I've updated the existing tests in obsconsumer to expect a downstream-wrapped error to exit the obsconsumer layer. I may add more tests later.

Documentation

None.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.45%. Comparing base (f68d710) to head (61ba663).
Report is 14 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13234   +/-   ##
=======================================
  Coverage   91.44%   91.45%           
=======================================
  Files         533      534    +1     
  Lines       29564    29596   +32     
=======================================
+ Hits        27034    27066   +32     
  Misses       1998     1998           
  Partials      532      532           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[mx-psi]

Is this ready for review now that the RFC amendment has been merged?

[jade-guiton-dd]

Not quite, I'm thinking of adding some additional tests. Not sure when I'll have the time to get to it though 😅

[jade-guiton-dd]

Looks like the contrib failures are due to #13364. This should be ready for review.

[mx-psi]

LGTM. @evan-bradley could you review the consumererror bits?

[jade-guiton-dd]

I think I've addressed your comments, could you take another look @evan-bradley?

[evan-bradley]

Looks good to me. Thanks for your patience with all my questions. 🙂

[jade-guiton-dd]

We discussed whether errors.Join(downstream, notDownstream) should be considered downstream or not during the Collector stability meeting (see this comment thread for context). Some points that were raised:

• A more idiomatic way to add context to a downstream error would be fmt.Errorf("error in <context>: %w", downstream), so it's not clear that we should support the "additional context" interpretation of errors.Join.
• If a component experiences an issue, but still succeeds in forwarding data to the next component, that issue was clearly not fatal to the processing of that payload. It should thus probably be surfaced as a warning log, not as an error returned to the caller with errors.Join. So it's not clear that we should support the "unrelated failure" interpretation of errors.Join either.
• Given that we can't really think of an idiomatic use case for this errors.Join call, we may want to consider it "undefined behavior", and default to the simplest implementation using errors.As. This is the current state of the PR.

I don't remember very well, but I think @dmitryax raised the point that perhaps we should instead default to the interpretation which is least likely to "hide" internal failures. Do you have objections to the current logic in the PR?

[mx-psi]

@open-telemetry/collector-approvers Based on the above comment I think we can merge this by EOW (after the merge conflicts are resolved) unless there are objections. If we explicitly consider this edge case to be unspecified behavior, I think we can go ahead with the choice we made