-
Notifications
You must be signed in to change notification settings - Fork 933
[Logs] Add optional Ergonomic API #4741
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| **Logger** - all methods are safe to be called concurrently. | ||
|
|
||
| ## Ergonomic API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably don't need a new section just to document something obvious (IMHO) to each language implementations. Languages were always free to offer extra helpers for all sort of things, and I think that is not prohibited in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to highlight a few points:
-
Issue Confusing purpose for logging API #4661 demonstrates real confusion - The contradiction in the opening paragraphs confused contributors. This suggests the freedom isn't as "obvious" as we might think. Moreover, based on the discussions during the specification SIG meeting, others also shared concerns in the current way the specification is written.
-
The section provides guidance, not just permission - It's not just saying "you can do this" (which may be implicit), but "here's what to focus on when you do this" (semconv compliance, idiomatic design).
-
Precedent exists - The Configuration spec explicitly states languages should provide idiomatic mechanisms. This follows that pattern.
-
Small documentation cost, significant clarity benefit - The section is brief (~5 lines) but resolves confusion that led to an issue being filed.
Would you prefer we:
- A) Keep the section as-is
- B) Move it to supplementary-guidelines.md instead
- C) Reduce it to a single sentence in the opening
- D) Something else?
I'm open to alternatives that achieve the clarity goal while addressing your concern about documenting the "obvious."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is : C
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with C. Let's see wait for more feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also updated the README.md so that this PR will be self-sufficient for solving the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During today's Spec SIG meeting it seemed that the attendees preference was: A
| applications, which is particularly important for: | ||
|
|
||
| - **Instrumentation libraries** to avoid coupling to a particular logging library. | ||
| - **Emitting structured logs and events** following [semantic conventions](https://opentelemetry.io/docs/specs/semconv/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is possible with logging libraries too. So may be specify that "this is the case when an existing logging library does not allow structure log or ability to specify event name"/similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed b10da75
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixes #4357
Fixes #4661
Changes
Add optional Ergonomic API that it is better suited for direct usage by instrumentation libraries, instrumented libraries, and applications.
Prototype
Go: https://github.com/pellared/olog