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

[Spike] sdk/log: Add Logger.Enabled and EnabledParameters #5816

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

pellared
Copy link
Member

@pellared pellared commented Sep 13, 2024

Fixes #5810

Remarks

Initially, I thought about keeping the feature go.opentelemetry.io/otel/sdk/log/internal/x.
However, I started to think if we should bring it back go.opentelemetry.io/otel/sdk as:

  1. Requiring the users to implement Enabled for all processors forces them to think how it should be handled (main reason). I feel that it is also more aligned with other interfaces in the SDK where usually we ask users to implement all possible methods.
  2. EnabledParameters would have to be defined in go.opentelemetry.io/otel/sdk/log/internal/x. Making it stable (moving to EnabledParameters would be a breaking change for people using the experimental feature which may be frustrating for them.
  3. I get a feeling that support for enabled in API and SDK would be stabilized on the spec level roughly at the same time. I asked a question here.
  4. I think this implementation easier to be used as a reference when proposing a PR for Specify how Logs SDK implements Enabled opentelemetry-specification#4207

I am open to other suggestions.

@pellared pellared changed the title Sdk enabled param sdk/log: Introduce EnabledParameters Sep 13, 2024
Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.4%. Comparing base (80e18a5) to head (f08ebd6).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5816     +/-   ##
=======================================
- Coverage   84.5%   84.4%   -0.1%     
=======================================
  Files        272     272             
  Lines      22776   22803     +27     
=======================================
+ Hits       19255   19260      +5     
- Misses      3178    3199     +21     
- Partials     343     344      +1     

see 7 files with indirect coverage changes

@pellared pellared changed the title sdk/log: Introduce EnabledParameters sdk/log: Introduce Logger.Enabled and EnabledParameters Sep 13, 2024
@pellared pellared changed the title sdk/log: Introduce Logger.Enabled and EnabledParameters sdk/log: Add Logger.Enabled and EnabledParameters Sep 13, 2024
@pellared pellared marked this pull request as ready for review September 13, 2024 06:31
@pellared pellared self-assigned this Sep 13, 2024
@pellared pellared added pkg:SDK Related to an SDK package enhancement New feature or request area:logs Part of OpenTelemetry logs labels Sep 13, 2024
@MrAlias
Copy link
Contributor

MrAlias commented Sep 13, 2024

2. EnabledParameters would have to be defined in go.opentelemetry.io/otel/sdk/log/internal/x. Making it stable (moving to EnabledParameters would be a breaking change for people using the experimental feature which may be frustrating for them.

It seems like this was the main motivation to make this change. I do not think it is a good one. It is not adding any value, it is only adding another layer of translation. I do not think we should make this change.

@pellared
Copy link
Member Author

pellared commented Sep 13, 2024

It is not adding any value, it is only adding another layer of translation

  1. As noted in the issue, it adds scope, resource, trace information which may be important for filtering (mainly instrumentation scope). This is the main motivation of this PR
  2. The type is immutable (safer to use by processors)

@MrAlias
Copy link
Contributor

MrAlias commented Sep 13, 2024

  1. it adds scope, resource, trace information which may be important for filtering (mainly instrumentation scope).

There is no justification provided to motivate this. It is computing these values and making the translation without having any evidence that is is desired.

Our only use-case right now is the minsev processor which does not need any of these values.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 13, 2024

  1. The type is immutable (safer to use by processors)

This indicates the API is not appropriately designed if this is desired. Should it be structured similar to the trace.SpanContext instead?

@pellared
Copy link
Member Author

There is no justification provided to motivate this

I pretty sure that there are going to be users that would want to filter out logs by logger name (instrumentatoon scope name).

I will do a PR in the spec on Monday to validate my opinion with others.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 13, 2024

There is no justification provided to motivate this

I pretty sure that there are going to be users that would want to filter out logs by logger name (instrumentatoon scope name).

I will do a PR in the spec on Monday to validate my opinion with others.

This mechanism is already defined in the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#loggerconfig

I don't anticipate adding a second way to configure this is going to be desired there, but that is just my opinion.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 13, 2024

  1. The type is immutable (safer to use by processors)

This indicates the API is not appropriately designed if this is desired. Should it be structured similar to the trace.SpanContext instead?

Alternate design proposal: #5820

@MrAlias
Copy link
Contributor

MrAlias commented Sep 13, 2024

  1. The type is immutable (safer to use by processors)

This indicates the API is not appropriately designed if this is desired. Should it be structured similar to the trace.SpanContext instead?

Alternate design proposal: #5820

Upon reflection, this alternate design proposal is not needed. The existing API passes EnabledParameters by value, not reference. Meaning the value each Logger implementation and log SDK Processor will get is already immutable.

There is no need to add another type to introduce immutability given this is already the case.

@pellared
Copy link
Member Author

pellared commented Sep 15, 2024

I don't anticipate adding a second way to configure this is going to be desired there, but that is just my opinion.

Gotcha. I feel that it is better to start with a smaller set of parameters. Of course this may change in future.

it is only adding another layer of translation.

Having a separate type in SDK gives the flexibility to decouple the Logger.Enabled and Processor.Enabled parameters from each other and makes it easier to remain compliant with the specification. It will also make the Logs SDK design more consistent as we already have separate definitions for Record in Logs API and SDK.

However, indeed it is better to discuss the specification before making further changes in the SDK to not go back and forth with the implementation. For now, the experimental design is usable.

@MrAlias, thanks for your feedback and research 🙏 Let me know if my plan to make a proposal in the spec first is a good idea.

@pellared pellared marked this pull request as draft September 15, 2024 21:28
@pellared pellared changed the title sdk/log: Add Logger.Enabled and EnabledParameters [Spike] sdk/log: Add Logger.Enabled and EnabledParameters Sep 17, 2024
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 enhancement New feature or request pkg:SDK Related to an SDK package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sdk/log: Introduce EnabledParameters
3 participants