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

[receiver/otlp] Refactor error handling into helper #9307

Conversation

TylerHelmuth
Copy link
Member

Description:
Moves reused code into helper function

Link to tracking Issue:
Closes #9300

Testing:
Added unit tests

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 17, 2024
@TylerHelmuth TylerHelmuth requested review from a team, djaglowski and mx-psi January 17, 2024 19:34
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4371e14) 90.35% compared to head (26aa379) 90.24%.
Report is 27 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9307      +/-   ##
==========================================
- Coverage   90.35%   90.24%   -0.12%     
==========================================
  Files         340      344       +4     
  Lines       17968    17996      +28     
==========================================
+ Hits        16235    16240       +5     
- Misses       1410     1428      +18     
- Partials      323      328       +5     

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

@TylerHelmuth TylerHelmuth force-pushed the otlpreceiver-refactor-error-handling branch from 5a83d85 to 6463b53 Compare January 17, 2024 19:39
receiver/otlpreceiver/internal/util/errors.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/internal/metrics/otlp.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/internal/trace/otlp.go Outdated Show resolved Hide resolved
Comment on lines 51 to 52
s := errors.GetStatusFromError(err)
return plogotlp.NewExportResponse(), s.Err()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
s := errors.GetStatusFromError(err)
return plogotlp.NewExportResponse(), s.Err()
return plogotlp.NewExportResponse(), errors.GetStatusFromError(err).Err()

Copy link
Member

Choose a reason for hiding this comment

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

If this is the case, why not always return the error from GetStatusFromError?

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning the entire Status makes GetStatusFromError more flexible for future use, but since there is not ab actual use case yet, and this is an internal package, I am ok with GetStatusFromError returning an error that made from a Status.

I don't feel strongly about this either way, so let me know what you all prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is an internal API I prefer to simplify usage, since everywhere we call .Err()

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 51 to 52
s := errors.GetStatusFromError(err)
return plogotlp.NewExportResponse(), s.Err()
Copy link
Member

Choose a reason for hiding this comment

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

If this is the case, why not always return the error from GetStatusFromError?

@bogdandrutu bogdandrutu merged commit 5ab066e into open-telemetry:main Jan 24, 2024
32 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 24, 2024
mx-psi pushed a commit that referenced this pull request Mar 27, 2024
…rrors (#9357)

**Description:**
Updates the receiver's http response to return a proper http status
based on whether or not the pipeline returned a retryable error. Builds
upon the work done in
#8080 and
#9307

**Link to tracking Issue:**

Closes
#9337
Closes
#8132
Closes
#9636
Closes
#6725

**Testing:**

Updated lots of unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/otlp] Unify error handling
4 participants