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 serialization IOException instead of rethrowing as runtime #6082

Merged
merged 7 commits into from
Jan 3, 2024

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Dec 18, 2023

Related to #6069. Alternative to #6076.

cc @ricardo-mestre

I verified the claim in #6076 by simulating IOExceptions while serializing. The threads in the OkHttp dispatcher thread pool were continually dying and being replaced due to IOExceptions being rethrown as RuntimeExceptions. This PR fixes that by adjusting the contract of HttpSender to accept a Marshaler instead of a Consumer<OutputStream>. While Consumer<OutputStream> was forced to catch exceptions, the Marshaler throws IOExceptions.

Its hard to recreate this behavior in a test, but I did verify in a contrived local setup that this change no longer results in the OkHttp dispatcher threads dying.

Note to @ricardo-mestre - this PR doesn't change the definition for which exceptions are retryable. That's defined here and needs followup in a separate PR. In contrast to OkHttpHttpSender, the JdkHttpSender currently retrying on almost all IOExceptions as defined here. Were able to do this because a user provided some great real-world data of all the different IOExceptions encountered and virtually all of them were retryable.

@jack-berg jack-berg requested a review from a team as a code owner December 18, 2023 21:01
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (517ef88) 90.95% compared to head (9a70faf) 90.99%.
Report is 2 commits behind head on main.

Files Patch % Lines
...ry/exporter/sender/jdk/internal/JdkHttpSender.java 75.00% 1 Missing and 1 partial ⚠️
...telemetry/exporter/internal/http/HttpExporter.java 50.00% 1 Missing ⚠️
...orter/sender/okhttp/internal/OkHttpHttpSender.java 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6082      +/-   ##
============================================
+ Coverage     90.95%   90.99%   +0.04%     
- Complexity     5682     5687       +5     
============================================
  Files           628      628              
  Lines         16672    16675       +3     
  Branches       1651     1654       +3     
============================================
+ Hits          15164    15174      +10     
+ Misses         1051     1047       -4     
+ Partials        457      454       -3     

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

@ricardo-mestre
Copy link
Contributor

Thanks a lot your active support with this one !

Comment on lines 88 to 90
CompletableResultCode result = exporter.export(new NoOpMarshaler(), 0);
result.join(1, TimeUnit.MINUTES);
Assertions.assertThat(result.isSuccess()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use awaitility for this, rather than result.join?

Something like await().untilAsserted(() -> Assertions.assertThat(result.isSuccess()).isTrue());

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in cases like this where the future is expected to resolve quickly and deterministically, its ok to use join. For example, we use it all over the place in AbstractHttpTelemetryExporterTest.

With that said, I'm actually not sure what this new test has to do with this change so I've gone ahead and removed it.

@jack-berg jack-berg added this to the 1.34.0 milestone Jan 3, 2024
@jack-berg jack-berg merged commit b4ed532 into open-telemetry:main Jan 3, 2024
30 of 32 checks passed
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

3 participants