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

Require severity in the EventLogger.builder() #6603

Open
bogdandrutu opened this issue Jul 25, 2024 · 1 comment
Open

Require severity in the EventLogger.builder() #6603

bogdandrutu opened this issue Jul 25, 2024 · 1 comment
Labels
blocked:spec blocked on open or unresolved spec Feature Request Suggest an idea for this project

Comments

@bogdandrutu
Copy link
Member

The main idea is that now, even if users sets a "filter" on events with severity lower than "error" the implementation of the EventLogger.builder() cannot return a "No-op" builder and has to record all the data until "build()" is called because the severity is not known.

It would be amazing to require severity to be provided as early as possible (with the event name) so that we can return a "no-op" builder when we are confident the event will be dropped, and not have to store attributes, etc.

@bogdandrutu bogdandrutu added the Feature Request Suggest an idea for this project label Jul 25, 2024
@jack-berg
Copy link
Member

We have experimental support for checking if a Logger or Tracer is enabled, and if a particular instrument is enabled (all instruments have APIs like this).

This seems like an extension of that request - support for the logger.isDebugEnabled() style of API, but for events.

The suggestion to accomplish by accepting a severity argument to the build is interesting, since it means you can accomplish this goal of avoiding unnecessary compute without requiring anything extra from the user. I.e. you can avoid the isDebugEnabled() call altogether.

It seems like it may be premature though, since there is nothing in the spec yet that suggests SDK capabilities around filtering based on log record severity. Some say you should configure this in the log framework when you're configuring it to bridge logs to opentelemetry - i.e. only configure logs with severity info or higher to be bridged. But this falls apart when you consider the event API since we want users to call it directly. Seems like the event SIG should consider whether severity should be a required arg as a prerequisite to stability, and push to evolve the log SDK to have the ability to filter based on severity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:spec blocked on open or unresolved spec Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

2 participants