Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat PartialSuccess as Success #9260

Merged
merged 4 commits into from Feb 6, 2024
Merged

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jan 10, 2024

Description: Changes the treatment of PartialSuccess, making them successful and logging a warning instead of returning an error to the caller. These responses are meant to convey successful receipt of valid data which could not be accepted for other reasons, specifically to cover situations where the OpenTelemetry SDK and Collector have done nothing wrong, specifically to avoid retries. While the existing OTLP exporter returns a permanent error (also avoids retries), it makes the situation look like a total failure when in fact it is more nuanced.

As discussed in the tracking issue, it is a lot of work to propagate these "partial" successes backwards in a pipeline, so the appropriate simple way to handle these items is to return success.

In this PR, we log a warning. In a future PR, (IMO) as discussed in open-telemetry/oteps#238, we should count the spans/metrics/logs that are rejected in this way using a dedicated outcome label.

Link to tracking Issue:
Part of #9243

Testing: Tests for the "partial success" warning have been added.

Documentation: PartialSuccess behavior was not documented. Given the level of detail in the README, it feels appropriate to continue not documenting, otherwise lots of new details should be added.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f5a7315) 0.00% compared to head (073ed55) 90.47%.

❗ Current head 073ed55 differs from pull request most recent head a14666b. Consider uploading reports for the commit a14666b to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           main    #9260       +/-   ##
=========================================
+ Coverage      0   90.47%   +90.47%     
=========================================
  Files         0      344      +344     
  Lines         0    18042    +18042     
=========================================
+ Hits          0    16323    +16323     
- Misses        0     1390     +1390     
- Partials      0      329      +329     

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

@jmacd jmacd marked this pull request as draft January 10, 2024 19:13
@jmacd
Copy link
Contributor Author

jmacd commented Jan 10, 2024

(Realize I need to fix the otlphttpexporter too!)

@jmacd jmacd marked this pull request as ready for review January 10, 2024 19:57
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exporter/otlpexporter/otlp.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Jan 19, 2024

Reviewers, this is ready for review.

@jmacd
Copy link
Contributor Author

jmacd commented Feb 2, 2024

Test failure looks unrelated:

Error: ./factory_test.go:142:3: unknown field GRPCClientSettings in struct literal of type otlpexporter.Config
Error: ./traces_test.go:39:3: unknown field GRPCClientSettings in struct literal of type otlpexporter.Config

@codeboten codeboten merged commit 52c914d into open-telemetry:main Feb 6, 2024
44 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants