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

Add an Enabled method to Logger #4995

Closed
MrAlias opened this issue Feb 29, 2024 · 9 comments · Fixed by #5071
Closed

Add an Enabled method to Logger #4995

MrAlias opened this issue Feb 29, 2024 · 9 comments · Fixed by #5071
Assignees
Labels
area:logs Part of OpenTelemetry logs blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made enhancement New feature or request pkg:API Related to an API package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Feb 29, 2024

While building out the slog bridge, it seems like having an Enabled method will be needed (originally proposed by @jba in #4809 (comment)).

From the bridge point of view if the log API requires it to respond to users with if the logging handler is going to actually log something for a context and severity level it needs to know what the implementation of the Logger will do.

It is not sufficient to track this in the bridge. If the bridge accepts some sort of minimum level things will be logged at it can be different than what the LoggerProvider that bridge also accepts is configured to minimally log. This is not a sufficient solution when the bridge needs to provide this information.

Additionally, when a bridge doesn't need to provide this information to the log API, it may still optimize its operation internally by checking if a log record is going to be recorded before calling Logger.Emit. This may be decent optimization for many bridges.

@MrAlias MrAlias added enhancement New feature or request blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Feb 29, 2024
@pellared pellared added pkg:API Related to an API package area:logs Part of OpenTelemetry logs labels Feb 29, 2024
@pellared
Copy link
Member

pellared commented Feb 29, 2024

This invalidates this rejected proposal https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#add-xyz-method-to-logger.

I have put the issue to GA project to at least discuss it before we moving it to post-GA.

@pellared
Copy link
Member

pellared commented Feb 29, 2024

This will be probably also handy for implementing following bridges:

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 29, 2024

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 29, 2024

This will be probably also handy for implementing following bridges:

Also, possibly, logrus: https://pkg.go.dev/github.com/sirupsen/logrus#Logger.IsLevelEnabled

@pellared
Copy link
Member

pellared commented Feb 29, 2024

If the bridge accepts some sort of minimum level things will be logged at

This was the idea

it can be different than what the LoggerProvider that bridge also accepts is configured to minimally log

As far as I remember, the SDK's logger provider configuration does not accept a severity level. Same for logger method. I think that without any of it adding Enabled to the Bridge API would not make much sense.

@pellared
Copy link
Member

For logrus I think the bridge would implement https://pkg.go.dev/github.com/sirupsen/logrus#Hook

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 29, 2024

What about No-Op Loggers?

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 29, 2024

I also think it should be added to the pipeline. If an exporter is only configured to export at a min level there needs to be a way to tell the SDK and user not to do things.

This is an operator concern, not one for instrumentation.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 1, 2024

Use cases from other languages

otel-rust added a similar method under feature flag - open-telemetry/opentelemetry-rust#1147
otel-cpp also has a similar approach - https://github.com/open-telemetry/opentelemetry-cpp/blob/07f6cb54ece56691dbd2a94b0cbeec722ff6a631/api/include/opentelemetry/logs/logger.h#L259

Originally posted by @lalitb in open-telemetry/opentelemetry-specification#3917 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made enhancement New feature or request pkg:API Related to an API package
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants