Add reminder to shutdown exporter instances if replacing #5688
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #5688 +/- ##
============================================
- Coverage 91.34% 91.34% -0.01%
Complexity 4997 4997
============================================
Files 554 554
Lines 14793 14793
Branches 1379 1379
============================================
- Hits 13513 13512 -1
Misses 883 883
- Partials 397 398 +1
☔ View full report in Codecov by Sentry. |
| * <p>NOTE: If returning a different exporter instance, be sure to call {@link | ||
| * SpanExporter#shutdown()} on the instance passed as an argument to release resources. |
There was a problem hiding this comment.
I think it's common to use this method to decorate the original
what do you think of putting this warning on toBuilder() instead?
There was a problem hiding this comment.
Is delegation a common pattern for this as well, in which case calling shutdown() would break the inner/underlying implementation right?
There was a problem hiding this comment.
Is delegation a common pattern for this as well
for toBuilder()? I think that makes a copy instead of using delegation
There was a problem hiding this comment.
The toBuilder() methods already have this warning, e.g. OtlpGrpcSpanExporter#toBuilder.
Was trying the belt and suspenders approach, but maybe unnecessary. How about I close this and re-open it if we get issues with resources not being cleaned up?
Improving docs now that #5680 and #5652 allow you to easily replace exporters with additional customizations not available via environment variables.