Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: modify LogExporter and TraceExporter interfaces to support returning failure #2381
base: main
Are you sure you want to change the base?
chore: modify LogExporter and TraceExporter interfaces to support returning failure #2381
Changes from 7 commits
2c24988
5b243a6
77d9a01
dc5d9b1
1b49188
a670a4e
6440a73
3c68efa
c2586ce
a460184
b1fff95
0ad193f
4df322f
26bbdc4
de49068
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 54 in opentelemetry-otlp/src/exporter/http/logs.rs
Codecov / codecov/patch
opentelemetry-otlp/src/exporter/http/logs.rs#L52-L54
Check warning on line 70 in opentelemetry-otlp/src/exporter/http/trace.rs
Codecov / codecov/patch
opentelemetry-otlp/src/exporter/http/trace.rs#L68-L70
Check warning on line 92 in opentelemetry-otlp/src/exporter/tonic/logs.rs
Codecov / codecov/patch
opentelemetry-otlp/src/exporter/tonic/logs.rs#L92
Check warning on line 94 in opentelemetry-otlp/src/exporter/tonic/logs.rs
Codecov / codecov/patch
opentelemetry-otlp/src/exporter/tonic/logs.rs#L94
Check warning on line 94 in opentelemetry-otlp/src/exporter/tonic/trace.rs
Codecov / codecov/patch
opentelemetry-otlp/src/exporter/tonic/trace.rs#L94
Check warning on line 96 in opentelemetry-otlp/src/exporter/tonic/trace.rs
Codecov / codecov/patch
opentelemetry-otlp/src/exporter/tonic/trace.rs#L96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still unsure of this....I can understand that a Provider's shutdown method, if invoked more than once, can return AlreadyShutdown error. How would an Exporter's shutdown be invoked more than once, since provider protects the processor/exporter from entering that situation?
I am not even sure if processor/exporter should even have a variable like
is_shutdown
at all.. If its shutdown is invoked, it'll kill the transports etc. any further attempt to export will result in failure naturally as transports are killed.(Sorry not really part of this PR, but this is something that I need some time to think through....)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel processor should have this flag. It can't assume how the exporters are implemented. Maybe exporter's shutdown methods are just no-op, and in that case the export may continue happening even after shutdown is invoked, if the processor doesn't have
is_shutdown
safeguard.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the need for processor to have the flag? If shutdown was already signaled, then no further shutdown signal will ever arrive at the LogProcessor, as the Provider takes care of that, and provider own Processors.
How would log processor ever get a 2nd shutdown call? Or how does it ever get an emit() call after shutdown is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes correct, is_shutdown at provider level will take care of that. I missed that. However, logger::emit() as of now doesn't check for the provider flag, which needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raised a PR to discuss this further - #2462
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This maybe okay. But if a user is calling shutdown more than once, it indicates there have an bug in the way they orchestrate telemetry pipelines, so isn't it best to let them know about it...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is logging it sufficient ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its debatable I guess....We can technically make almost every return type go away in favor of logging. So need to find a balance where do we resort to logging vs explicit-return-type.
Related: Otel .NET decided to specialy handle emit logs after shutdown, by printing these nice message only in StdOut exporter:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs#L39-L42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon there’s a clear line there between errors that actually reflect some failure of the system, and errors that have no impact and are just hinting that the caller has done something funny.
By way of example a flush that fails become the connections gone is very different to “probably you didn’t mean to call this twice”.
That stdout error one is interesting. Is there a story there ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also and regardless - it feels like something we should have a consistent view on - when to error, when not to - that we can point at apply uniformly. That consistency is maybe even more important than what the policy itself is 🤷♂️
Check warning on line 134 in opentelemetry-sdk/src/export/trace.rs
Codecov / codecov/patch
opentelemetry-sdk/src/export/trace.rs#L132-L134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure if we should make these public.. There are strictly internal details, and a mutex poison is not user actionable at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were public before in the old type; but I agree with you!
I can hide it behind the GenericError or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I am missing some discussion - why do we need to have new error enum for shutdown. Can't we reuse the
LogError
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep one uber-error and the caller wants to handle it, they are forced to handle variants that have nothing to do with the operation they are calling
Check warning on line 81 in opentelemetry-sdk/src/logs/error.rs
Codecov / codecov/patch
opentelemetry-sdk/src/logs/error.rs#L79-L81
Check warning on line 656 in opentelemetry-sdk/src/logs/log_processor.rs
Codecov / codecov/patch
opentelemetry-sdk/src/logs/log_processor.rs#L655-L656
Check warning on line 659 in opentelemetry-sdk/src/logs/log_processor.rs
Codecov / codecov/patch
opentelemetry-sdk/src/logs/log_processor.rs#L659
Check warning on line 661 in opentelemetry-sdk/src/logs/log_processor.rs
Codecov / codecov/patch
opentelemetry-sdk/src/logs/log_processor.rs#L661
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should (in another issue!) chase this distinction between
TraceError
andShutdownError
further down the rabbit hole. For the same reasons I don't think it makes sense to use an über-error in the exporter traitCheck warning on line 432 in opentelemetry-sdk/src/trace/span_processor.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/span_processor.rs#L432
Check warning on line 435 in opentelemetry-sdk/src/trace/span_processor.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/span_processor.rs#L435