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.

Sign up for GitHub

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

Jump to bottom

Is LogEmitter.flush necessary? #2342

Closed
tigrannajaryan opened this issue Feb 14, 2022 · 3 comments · Fixed by #2405
Closed

Is LogEmitter.flush necessary? #2342

tigrannajaryan opened this issue Feb 14, 2022 · 3 comments · Fixed by #2405
Assignees
@srikanthccv
Labels
spec:logs Related to the specification/logs directory

Comments

@tigrannajaryan
Copy link
Member

Is LogEmitter.flash necessary or it is sufficient to have LogEmitterProvide.ForceFlush?

See also discussion in Python prototype which currently calls LogEmitter.flash.

Is LogEmitterProvider.ForceFlush accessible at the call sites that currently call LogEmitter.flash? If yes then we can delete LogEmitter.flash.
@tigrannajaryan tigrannajaryan added the spec:logs Related to the specification/logs directory label Feb 14, 2022
@tigrannajaryan tigrannajaryan mentioned this issue Feb 14, 2022
Merged
@anuraaga anuraaga changed the title Is LogEmitter.flash necessary? Is LogEmitter.flush necessary? Feb 15, 2022
@srikanthccv
Copy link
Member

Is LogEmitterProvider.ForceFlush accessible at the call sites that currently call LogEmitter.flash

Yes, this can access the global log emitter provider and directly call on provider flush. I see there is another open issue #2332 if we should have the global provider set/get. If we decide to not allow that then I think python sdk should update the Handler (analogous to appender) to be initialised with a provider.

@tigrannajaryan
Copy link
Member Author

@srikanthccv will you be able to take this issue and submit a PR to fix this in the spec?

@srikanthccv
Copy link
Member

Sure, please assign to me.

@srikanthccv srikanthccv mentioned this issue Mar 6, 2022
Merged
tigrannajaryan pushed a commit that referenced this issue Mar 21, 2022
@srikanthccv
Remove flush method on LogEmitter (#2405)
441bafa 
Fixes #2342 

## Changes

This change proposes to remove the `flush` on Emitter for the consistency. There is no `flush` on Tracer/Meter specification. Language sdk implementations may choose the idiomatic way to provide the appender/handler ability to call flush on the `LogEmitterProvider` (ex: setter in [java](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.16/library/src/main/java/io/opentelemetry/instrumentation/log4j/appender/v2_16/OpenTelemetryAppender.java#L86-L95), `init` arg in py, or possibly a global accessor for provider etc...)
@pranavmarla pranavmarla mentioned this issue Jul 26, 2022
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
@srikanthccv
Remove flush method on LogEmitter (open-telemetry#2405)
1e944ec 
Fixes open-telemetry#2342 

## Changes

This change proposes to remove the `flush` on Emitter for the consistency. There is no `flush` on Tracer/Meter specification. Language sdk implementations may choose the idiomatic way to provide the appender/handler ability to call flush on the `LogEmitterProvider` (ex: setter in [java](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.16/library/src/main/java/io/opentelemetry/instrumentation/log4j/appender/v2_16/OpenTelemetryAppender.java#L86-L95), `init` arg in py, or possibly a global accessor for provider etc...)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@srikanthccv srikanthccv

Labels
spec:logs Related to the specification/logs directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove flush method on LogEmitter srikanthccv/opentelemetry-specification
3 participants
@tigrannajaryan @SergeyKanzhelev @srikanthccv