Cleanup autoconfigured resources in case of exception #5117
Conversation
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These testsuites were split out because they left behind resources that causes issues with other tests. Being able to merge them into the other test suites was a decent proxy for confirming that all tests were properly cleaning up resources.
| SpanLimits config = | ||
| TracerProviderConfiguration.configureSpanLimits( | ||
| LogLimits config = | ||
| LoggerProviderConfiguration.configureLogLimits( |
There was a problem hiding this comment.
A test bug from bad copy / paste.
| @Test | ||
| @SetSystemProperty(key = "otel.propagators", value = "cat") | ||
| void invalidPropagator() { | ||
| // TODO(jack-berg): confirm log warnings go away once exporters are properly shutdown (#5113) |
There was a problem hiding this comment.
There's misleading log messages in the test output that look like this:
[Test worker] ERROR io.opentelemetry.exporter.internal.grpc.OkHttpGrpcExporter - Failed to export metrics. The request could not be executed. Full error message: executor rejected
[Test worker] ERROR io.opentelemetry.exporter.internal.grpc.OkHttpGrpcExporter - Failed to export metrics. The request could not be executed. Full error message: executor rejected
They can happen when a test like this initializes a periodic metric reader with an exporter, and the exporter is shutdown before the periodic metric reader. When the periodic metric reader is shutdown it flushes to the exporter which has already had its executor threads shutdown. #5113 fixes this by adjusting all exporters to reject exports after being shutdown. This really is the correct behavior trying to export is bound to produce various exceptions after the network resources have been closed.
There was a problem hiding this comment.
Are you intending to keep the TODO here? Do we want to wait until 5113 is in the can?
There was a problem hiding this comment.
Removed since #5113 is merged!
|
|
||
| try { | ||
| try (SdkLoggerProvider loggerProvider = builder.build()) { |
There was a problem hiding this comment.
This type of change is a common theme in this PR. Switching to try-with-resources when available to cleanup the code, and also block (for at most 10 seconds) until the resource is closed.
|
|
||
| @Test | ||
| void configuration() { | ||
| void configureExporter_spiExporter() { |
There was a problem hiding this comment.
While I'm in the neighborhood, I've renamed a bunch of vaguely named tests to better describe what they do.
Codecov ReportBase: 91.13% // Head: 91.14% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #5117 +/- ##
=========================================
Coverage 91.13% 91.14%
- Complexity 4907 4911 +4
=========================================
Files 549 547 -2
Lines 14506 14544 +38
Branches 1393 1399 +6
=========================================
+ Hits 13220 13256 +36
- Misses 890 891 +1
- Partials 396 397 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I added an additional commit which reduces the log level when exporters are shutdown multiple times from Reducing the log level to |
Resolves #5090.
Wraps entire autoconfigure code block in a try / catch. Adds all
Closeablecomponents to aList<Closeable>. If an exception is thrown, catch it and close all the closeables in the list, then rethrow the exception.