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 EventName parameter to Logger.Enabled #4220

Closed
pellared opened this issue Sep 20, 2024 · 8 comments
Closed

Add EventName parameter to Logger.Enabled #4220

pellared opened this issue Sep 20, 2024 · 8 comments
Labels
sig-issue A specific SIG should look into this before discussing at the spec

Comments

@pellared
Copy link
Member

pellared commented Sep 20, 2024

There is a an open question whether EventName (or EventID) should be added as an optional parameter to Logger.Enabled.

Regd. EventName being used for EnabledCheck:

  1. It is very important (actually the EventId, the numerical, machine friendly version of EventName that is most important to do ultra fast checks!) for scenarios we are working on in OTel Rust, C++. I don't know if it is something every language/implementation cares about. Given spec does not prohibit an implementation from allowing more parameters, I am totally okay if spec does not mention it, as OTel C++/Rust can offer this as extras.
  2. From the Event Oteps, there were mention of scenarios where Loggers can filter based on EventName, route things to different places based on EventName etc. It'd be good to wait to see the progress on Events before we can really say if EventName is a important parameter for the Enabled check.

Originally posted by @cijothomas in #4203 (comment)

@pellared
Copy link
Member Author

pellared commented Sep 20, 2024

Notes from 2023-09-20 Events/Logs SIG meeting:

@lmolkova, pointed out that if we would like to filter by event name then we would need to store a map (set) of disabled (or enabled) event names which can be inefficient (memory overhead).

@MSNev, had a remark that it is better to filter out events by severity rather than by name.

@pellared, said that on top of that each Logger can have its own severity threshold so each instrumentation library can have a distinct severity level.

@cijothomas, agreed that probably it is indeed better to keep the parameters small and just accept context and severity.

I am planning to discuss it during the next Specification SIG meeting. If there will be no objections then I plan to close this issue. We can reopen it in future if needed.

@pellared
Copy link
Member Author

Spec SIG meeting notes:
The event name may be added before stabilizing the API.

@svrnm svrnm added the triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted label Sep 30, 2024
@pellared pellared added this to Logs SIG Oct 2, 2024
@pellared
Copy link
Member Author

pellared commented Oct 2, 2024

I am against this proposal as I do not think that handling event.name for Enabled is more important than for other attributes. Passing all attributes "destroys" the idea of Enabled which is supposed to be used by the user to check whether it makes sense to build and emit a log records (performance tuning). The parameters accepted by Logger.Enabled should be "cheap" to construct.

The SDK's Enabled implementation would get the context, severity level, instrumentation scope that should be enough.

Side note: I think that instrumentation libraries emitting events could add a event.namespace instrumentation scope attribute instead of putting the namespace to each event.name value as a prefix. EDIT: I created #4239

@trask
Copy link
Member

trask commented Oct 2, 2024

I am against this proposal as I do not think that handling event.name for Enabled is more important than for other attributes.

I'm not sure I agree, but even if we decide we want event name, I think it would be fine (maybe even better?) to introduce a new method EventEnabled (possibly taking both event name and severity), which I think would remove Go's worry about introducing a new parameter to the Enabled method(?)

@pellared
Copy link
Member Author

pellared commented Oct 2, 2024

As for Go SIG we are not worried about adding a parameter in future. We decided to favor future-compatibilty instead of slightly better ergonomics.

I am not sure if this issue should be a blocker for stabilizing Logger.Enabled.

@cijothomas
Copy link
Member

I am against this proposal as I do not think that handling event.name for Enabled is more important than for other attributes. Passing all attributes "destroys" the idea of Enabled which is supposed to be used by the user to check whether it makes sense to build and emit a log records (performance tuning). The parameters accepted by Logger.Enabled should be "cheap" to construct.

The SDK's Enabled implementation would get the context, severity level, instrumentation scope that should be enough.

Side note: I think that instrumentation libraries emitting events could add a event.namespace instrumentation scope attribute instead of putting the namespace to each event.name value as a prefix. EDIT: I created #4239

If "event.name" is an attribute, then I totally agree!
(Which is why OTel .NET, C++, Rust all decided to put EventName as a top-level field in LogRecord, and not inside Attributes)

@github-actions github-actions bot added the triage:followup Needs follow up during triage label Oct 15, 2024
@pellared pellared changed the title Add EventName parameter to Logger.Enabled [logs] Add EventName parameter to Logger.Enabled Oct 15, 2024
@pellared pellared changed the title [logs] Add EventName parameter to Logger.Enabled Add EventName parameter to Logger.Enabled Oct 15, 2024
@trask trask added sig-issue A specific SIG should look into this before discussing at the spec and removed triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted triage:followup Needs follow up during triage labels Oct 15, 2024
@pellared
Copy link
Member Author

It is possible that it will be a parameter for a dedicated enabled method for events (and not for log records):

@pellared
Copy link
Member Author

Closing per #4220 (comment) and #4220 (comment)

@pellared pellared closed this as not planned Won't fix, can't repro, duplicate, stale Nov 26, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Logs SIG Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig-issue A specific SIG should look into this before discussing at the spec
Projects
Status: Done
Development

No branches or pull requests

4 participants