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

Stabilize Logger.Enabled #4208

Open
pellared opened this issue Sep 10, 2024 · 19 comments
Open

Stabilize Logger.Enabled #4208

pellared opened this issue Sep 10, 2024 · 19 comments
Assignees
Labels
maintainer-request Escalated by SIG maintainers spec:logs Related to the specification/logs directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned

Comments

@pellared
Copy link
Member

pellared commented Sep 10, 2024

Stabilize Logger.Enabled API

Blockers:

@pellared pellared added the spec:logs Related to the specification/logs directory label Sep 10, 2024
@pellared pellared moved this from Todo to Blocked in Go: Logs (GA) Sep 10, 2024
@pellared
Copy link
Member Author

Question to @open-telemetry/technical-committee: Do we want to stabilize the Logger.Enabled API sooner than we stabilize the spec defining how SDK implements it? Or do we want to stabilize Enabled for API and SDK at the same time?

@trask trask added the triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted label Sep 17, 2024
@cijothomas
Copy link
Member

Question to @open-telemetry/technical-committee: Do we want to stabilize the Logger.Enabled API sooner than we stabilize the spec defining how SDK implements it? Or do we want to stabilize Enabled for API and SDK at the same time?

Same for Metrics too: https://github.com/open-telemetry/opentelemetry-specification/pull/4219/files#r1767789558

@pellared pellared changed the title Stabilize Logs Enabled Stabilize Logger.Enabled Oct 1, 2024
@github-actions github-actions bot added the triage:followup Needs follow up during triage label Oct 2, 2024
@pellared pellared added this to Logs SIG Oct 2, 2024
@pellared
Copy link
Member Author

pellared commented Oct 2, 2024

The lack of stabilization of Logger.Enabled API blocks stabilization of OTel Go Logs.
Logger.Enabled API is required for bridging most popular Go logging libraries (including slog from the Go standard library).

From OTel Go perspective, the SDK support can be experimental. See: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log/internal/x.

This is currently the only known blocker for stabilizing the OTel Go Logs.

@pellared
Copy link
Member Author

pellared commented Oct 3, 2024

@open-telemetry/technical-committee, are you able to revalidate if the issues listed as blockers are still seen as blockers or if they can be addressed after stabilization of Logger.Enabled in Logs Bridge API?

Personally, I think the main blocker is to have at least 3 prototypes of the API in different languages.

@danielgblanco danielgblanco added triage:deciding:tc-inbox Needs attention from the TC in order to move forward 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 7, 2024
@pellared pellared self-assigned this Oct 15, 2024
@tigrannajaryan tigrannajaryan self-assigned this Oct 16, 2024
@tigrannajaryan tigrannajaryan removed the triage:deciding:tc-inbox Needs attention from the TC in order to move forward label Oct 16, 2024
@tigrannajaryan
Copy link
Member

To clarify the process: we expect 3 prototypes in 3 different languages that can be used by the end users, so that they can try the feature, provide feedback, submit bugs and issues about it. This is a necessary process before the spec section is marked "Stable".

From this perspective a PR does not counts as a prototype since it is not easily usable by the end users. A PR is fine for proposing new experimental features and demonstrating how they would work, but it is not enough for stabilizing the spec.

The lack of stabilization of Logger.Enabled API blocks stabilization of OTel Go Logs.
Logger.Enabled API is required for bridging most popular Go logging libraries (including slog from the Go standard library).

@pellared you either need to find a way to have unstable APIs in Go or wait until other languages implement the prototypes. Either way the ability to have unstable APIs is very valuable and this is likely to come up again as Otel evolves and we keep adding new experimental APIs to existing signals.

--

As a side note: I encourage using maturity levels between "Development" and "Stable" to signal increasing level of confidence in the capability (both in the spec and the SDK). For example if we have 1-2 prototypes then we can move the maturity level of the feature from "Development" to "Alpha" or "Beta" to signal it is moving closer to the "Stable" state.

@pellared
Copy link
Member Author

pellared commented Oct 16, 2024

OK. See we need 3 different languages to have it released as experimental API.

Here is how the experimental Logger.Enabled API is currently defined in a 3 languages:

I will do my best to work on this with others to move this forward (as we have inconsistencies).

@pellared you either need to find a way to have unstable APIs in Go

All major log bridges need it so it does not even make sense to stabilize the rest as the Logs API would not be usable in Go ecosystem. From #3917:

Multiple logging libraries in Go provide this optimization1234. If the Go SIG is going to be able to support these critical logging systems we need this functionality in the Logs Bridge API.

or wait until other languages implement the prototypes.

We then need to wait for other languages to add it.

As a side note: I encourage using maturity levels between "Development" and "Stable" to signal increasing level of confidence in the capability (both in the spec and the SDK)

I am not sure if we can use Alpha and Beta in the spec documents as these are not defined in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/document-status.md#lifecycle-status.

Footnotes

  1. https://pkg.go.dev/log/slog#Logger.Enabled

  2. https://pkg.go.dev/go.uber.org/zap#Logger.Level

  3. https://pkg.go.dev/github.com/go-logr/logr#LogSink

  4. https://pkg.go.dev/github.com/sirupsen/logrus#Logger.IsLevelEnabled

@tigrannajaryan
Copy link
Member

I am not sure if we can use Alpha and Beta in the spec documents as these are not defined in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/document-status.md#lifecycle-status.

We can bring as many levels from OTEP 0232 to the spec as we will believe is useful. I started with 3 but we can bring more if we feel there is value. I personally think it can be valuable to have more granularity between Development (the most immature) and Stable (the most mature). It is an important signal and having just a binary value for it I think is not nuanced enough. Stabilization is a process, often a long one at Otel. As you move along that process it is important to indicate the progress by updating the level labels.

@MrAlias
Copy link
Contributor

MrAlias commented Oct 17, 2024

From this perspective a PR does not counts as a prototype since it is not easily usable by the end users. A PR is fine for proposing new experimental features and demonstrating how they would work, but it is not enough for stabilizing the spec.

FWIW this is a change in policy. Many features have been stabilized that relied on "Go implementations" which were just PRs.

I'm not sure it is fair to make this change in policy in such an ad hoc manner.

@tigrannajaryan
Copy link
Member

@MrAlias my post is a result of a discussion by a few TCs while triaging this issue, so it is not an official policy change yet.

I tried finding the current policy but couldn't. This document does not seem to have an opinion about what criteria must be met before spec stabilization.

If anyone is aware of where do we state how many prototypes are needed please post the link.

If the policy does not exist in written form or we need to modify it I will create an issue so that we can discuss and formalize it.

Let's keep this issue open for now so that we can apply consistent rules after we clarify what the rules are.

@tigrannajaryan
Copy link
Member

I am gonna move this back to TC inbox.

@tigrannajaryan tigrannajaryan added the triage:deciding:tc-inbox Needs attention from the TC in order to move forward label Oct 17, 2024
@jack-berg jack-berg removed the triage:deciding:tc-inbox Needs attention from the TC in order to move forward label Oct 23, 2024
@jack-berg
Copy link
Member

Removed from TC inbox. The prototype requirement is being separately tracked, and there are other blockers preventing stability.

@jpkrohling jpkrohling added the triage:followup Needs follow up during triage label Nov 4, 2024
@jpkrohling
Copy link
Member

Labeling with follow-up, as I understand we need the 3 implementations before this can be declared stable.

@trask
Copy link
Member

trask commented Nov 5, 2024

removing triage:followup since the automation will automatically add it back 2 weeks after additional comments / ref links

@trask trask removed the triage:followup Needs follow up during triage label Nov 5, 2024
@svrnm svrnm added the triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned label Nov 25, 2024
@pellared pellared added the maintainer-request Escalated by SIG maintainers label Jan 21, 2025
@pellared
Copy link
Member Author

pellared commented Jan 22, 2025

@jack-berg, I have question on what is required regarding the SDK implementation specification/design to unblock the stabilization of Logger.Enabled API.

  1. Is an OTEP like this good enough: OTEP: Logger.Enabled #4290?
  2. Or maybe addressing is good enough: Add min_severity to LoggerConfig #4364 would be good enough?
  3. Or both of them are required (OTEP + some experimental feature in specification)?
  4. Something else?

From our OTel Go experience, it is better to stabilize the API first and then gather feedback for months before stabilizing anything in the SDK. In OTel Go all our logging bridges use Logger.Enabled and we have an experimental support for adding hooks on the SDK side (as described in the OTEP). Nobody reported any issue or negative feedback since a few months.

The stabilization of Logger.Enabled on the API side is one of our main blockers for releasing a stable OTel Go Logs API and SDK.

@pellared
Copy link
Member Author

pellared commented Jan 22, 2025

I also want to call out that we already have 4 working prototypes of Logger.Enabled:

They have slight differences in the accepted parameters and I think this needs to be sorted out.

@open-telemetry/cpp-maintainers, @open-telemetry/php-maintainers, @open-telemetry/rust-maintainers, I need your help here.

@jack-berg
Copy link
Member

@jack-berg, I have question on what is required regarding the SDK implementation specification/design to unblock the stabilization of Logger.Enabled API.

Honestly, I've been struggling to keep up with / track all the open issues / PRs related to this.

The current state of the operation is that enabled accepts context and severity number parameters. My original critique was that we shouldn't let API drift away, or stabilize, without corresponding features in the SDK. We could of overlook this requirement, given that the usefulness of the parameters seems pretty obvious, but I think doing so would set a bad precedent so I would vote against it.

Your OTEP #4290 tries to establish this corresponding SDK behavior. I agree with some of the content in there, and in particular, think that adding LoggerConfig#min_severity and LoggerConfig#trace_based properties are no brainers. I wonder if anyone would disagree? If not, then adding those two properties (along with extending #4381 to reference them) seems like the easiest way forward to me.

I also want to call out that we already have 4 working prototypes of Logger.Enabled:

Five - I originally proposed this experimental method with an accompanying java prototype 🙂

@trask
Copy link
Member

trask commented Jan 22, 2025

adding LoggerConfig#min_severity and LoggerConfig#trace_based properties are no brainers. I wonder if anyone would disagree? If not, then adding those two properties (along with extending #4381 to reference them) seems like the easiest way forward to me.

👍

@brettmc
Copy link
Contributor

brettmc commented Jan 22, 2025

I also want to call out that we already have 4 working prototypes of Logger.Enabled

PHP's implementation of enabled hasn't caught up to more recent spec changes, so is based on https://github.com/open-telemetry/opentelemetry-specification/blob/v1.34.0/specification/logs/bridge-api.md#enabled

When we do catch up, I'll follow the latest spec (although I might wait for this effort to complete first)

@pellared
Copy link
Member Author

in particular, think that adding LoggerConfig#min_severity and LoggerConfig#trace_based properties are no brainers. I wonder if anyone would disagree?

I do not see them as no brainers. I am personally not convinced that these are good proposals.

Regarding LoggerConfig#min_severity: #4290 (comment)

Regarding LoggerConfig#trace_based: #4290 (comment)

I find adding opt-in Enabled to LogRecordProcessor more flexible, cohesive, composable. I think this is the route I will try to pursue even though I am aware that it will be harder for me, I think this is simply a better design and the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer-request Escalated by SIG maintainers spec:logs Related to the specification/logs directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
Status: Blocked
Status: Todo
Status: No status
Development

No branches or pull requests

10 participants