search

open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.76k stars 890 forks source link

Define Enabled parameters for Logger #4203

Closed pellared closed 2 months ago

pellared commented 2 months ago

Follows https://github.com/open-telemetry/opentelemetry-specification/pull/4020

During 7/5/24 OTel Specification SIG meeting it was decided to move the discussion about parameters to a separate PR; a little cleaned up transcript:

We're essentially trying to foresee with this is like what arguments will need to be present, for this Enabled method. Is it just severity? Is it no arguments at all? Are we going to want to evolve the arguments over time. I don't have the answers to those questions. I wonder if we can do something [...] just to progress this because there's kind of [...] chicken in the eggs problems here. [...] we want to get this merged. [...] This API should be structured so that the arguments you pass to it are evolvable. [...] we can leave ourselves room to evolve it from multiple standpoints. There's explicit text that says that the arguments can change and it's experimental. I think I'd feel good about it then, because then we could [...] get this valuable thing in now [...] [...] And maybe we open some sort of issue [...] as a prerequisite for stability, we should [...] more thoughtfully consider what arguments are required for this.

This is needed for stabilization of Logger.Enabled in the spec which is need for the stabilization of Go Logs API.

Related issue: https://github.com/open-telemetry/opentelemetry-go/issues/5769

Implementation in Go: https://github.com/open-telemetry/opentelemetry-go/pull/5791

Changes

Add a minimal (and maybe even sufficient?) set of parameters for Logger.Enabled:

Add guidelines/rules for the Enabled parameters.

pellared commented 2 months ago

In a nutshell, my critique is that the SDK has to provide an implementation of this Logger#enabled method, but there is currently nothing in the SDK spec which would allow the Logger#enabled to do anything differently based on the context / severity arguments.

Cannot this be proposed as a separate PR?

In Go SIG, we are indeed experimenting how to best model it on the SDK level. We have an experimental feature, but we would like to have some more feedback before we propose something to the specification.

I think we can separately work on specifying the API and how SDK is implementing the API.

In my opinion, designing and implementing the SDK implementation would be harder and more opinionated. Therefore, I think it would be better to separate those into distinct PRs.

I agree that not having any specification on the SDK level may be seen as a blocker of making it stable. On the other hand, there are already APIs which are not defined how the SDK should handle them like https://github.com/open-telemetry/opentelemetry-specification/issues/4160.

Notice that this section is still in "Development".

jack-berg commented 2 months ago

Cannot this be proposed as a separate PR?

I'm fine splitting this out in a way that allows iteration. But we need a way to not loose track of this - maybe a TODO in the spec and a new issue upon merging? Resolving this discrepancy between the API and SDK should be a blocker to stability.

pellared commented 2 months ago

Cannot this be proposed as a separate PR?

I'm fine splitting this out in a way that allows iteration. But we need a way to not loose track of this - maybe a TODO in the spec and a new issue upon merging? Resolving this discrepancy between the API and SDK should be a blocker to stability.

Good idea. I created:

Feel free to update description or even create more issues.

trask commented 2 months ago

may also want to consider blocking stability on whether we want to include "event name" in the enabled parameters, e.g. see @lmolkova's https://github.com/open-telemetry/oteps/pull/265#issuecomment-2344112029

pellared commented 2 months ago

may also want to consider blocking stability on whether we want to include "event name" in the enabled parameters, e.g. see @lmolkova's open-telemetry/oteps#265 (comment)

If you want you can add it to https://github.com/open-telemetry/opentelemetry-specification/issues/4208.

However, I do not think that it should block the stability of Enabled as:

  1. Adding a parameter would not be a breaking change
  2. I feel that there is no conclusion if we want to add an "event name" do the LogRecord model or if we just want to use an event.name log attribute for it. It may be better to add "attributes" to enabled parameters. I think we would rather use just an attribute for that. See https://github.com/open-telemetry/oteps/pull/265#issuecomment-2339082984
reyang commented 2 months ago
  1. Adding a parameter would not be a breaking change

💯

  1. I feel that there is no conclusion if we want to add an "event name" do the LogRecord model or if we just want to use an event.name log attribute for it. It may be better to add "attributes" to enabled parameters. I think we would rather use just an attribute for that. See Event basics oteps#265 (comment)

There might be more to consider (definitely not for this PR):

  1. Comparing strings could be very inefficient, many systems use integers (e.g. Event Id = 1001) in combination with user-friendly names.
  2. For platforms with very limited resource or mission critical software (e.g. IoT device, kernel drivers), folks might want to just emit the integer, then enrich the data in the pipeline/backend using the integer -> string name mapping.
pellared commented 2 months ago

Here (Proposal A) is an idea how the SDK can implement Logger.Enabled.

pellared commented 2 months ago

Here (Proposal B) is a second (I think better) idea how the SDK can implement filtering which will affect not only Logger.Enabled but also Logger.Emit. It is more like sampling in tracing SDK.