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

Propagate non-retryable error messages to client #5929

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mark-pictor-csec
Copy link
Contributor

@mark-pictor-csec mark-pictor-csec commented Oct 29, 2024

PR #5541 (and issue #5536) enhance error handling, returning body text as part of the error. However, this is only done for retryable errors; if non-retryable, error text still does not propagate to clients.

This PR adds handling of non-retryable errors, ensuring any body text is part of the message returned to the user's code. There is no change to the circumstances under which errors are reported, just an enhancement of the content of such an error.

Copy link

linux-foundation-easycla bot commented Oct 29, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu dmathieu added the blocked:CLA Waiting on CLA to be signed before progress can be made label Oct 30, 2024
@dmathieu
Copy link
Member

Aside from these errors, I noticed inconsistencies in the closing of the response body. For traces, the Close happened in a defer statement and any error was logged. Logs and metrics were less rigorous. It appeared Close() wasn't always called, and when it was, errors were returned sometimes and ignored at other times.

Could you split that into another PR?

You will need to sign the CLA.

@mark-pictor-csec
Copy link
Contributor Author

Aside from these errors, I noticed inconsistencies in the closing of the response body. For traces, the Close happened in a defer statement and any error was logged. Logs and metrics were less rigorous. It appeared Close() wasn't always called, and when it was, errors were returned sometimes and ignored at other times.

Could you split that into another PR?

Can do.

You will need to sign the CLA.

Yes, I am waiting on someone within my company (corp CLA).

@mark-pictor-csec mark-pictor-csec marked this pull request as draft October 31, 2024 19:35
@mark-pictor-csec
Copy link
Contributor Author

mark-pictor-csec commented Oct 31, 2024

Converted to draft until I get the CLA issue resolved.
Now waiting on the body close PR #5954 since this depends upon it.

@mark-pictor-csec mark-pictor-csec force-pushed the propagate-client-errors branch 2 times, most recently from 5c6dc2a to 4c276b1 Compare November 6, 2024 22:37
pellared pushed a commit that referenced this pull request Nov 12, 2024
There were inconsistencies in closing the response body. For traces, the
Close happened in a defer statement and any error was logged. Logs and
metrics were less rigorous. It appeared Close() wasn't always called,
and when it was, errors were returned sometimes and ignored at other
times.

This applies the defer logic from traces to the other two and removes
other Close() calls.

This was part of PR #5929, and has been split out as
requested:
#5929 (comment)).
PR open-telemetry#5541 (and issue open-telemetry#5536) enhance error handling, returning body text
as part of the error. However, this is only done for retryable errors;
if non-retryable, error text still does not propagate to clients.

This PR adds handling of non-retryable errors, ensuring any body text is
part of the message returned to the user's code. There is no change to
the circumstances under which errors are reported, just an enhancement
of the content of such an error.
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.7%. Comparing base (d428313) to head (ffa1610).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5929   +/-   ##
=====================================
  Coverage   84.6%   84.7%           
=====================================
  Files        272     272           
  Lines      22897   22906    +9     
=====================================
+ Hits       19391   19403   +12     
+ Misses      3162    3159    -3     
  Partials     344     344           

see 4 files with indirect coverage changes

@mark-pictor-csec mark-pictor-csec marked this pull request as ready for review November 12, 2024 19:05
@mark-pictor-csec
Copy link
Contributor Author

@dmathieu CLA signed and the PR split from this is merged. Shall I create a new PR or are we good to go with this one?

@pellared
Copy link
Member

we good to go with this one?

We are good with this one.

@pellared pellared removed the blocked:CLA Waiting on CLA to be signed before progress can be made label Nov 13, 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.

3 participants