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.

Sign up for GitHub

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

Jump to bottom

Do not require extensible Logger.Enabled parameters list #4221

Merged
merged 8 commits into from
Oct 1, 2024
Merged

Do not require extensible Logger.Enabled parameters list #4221

merged 8 commits into from
Oct 1, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Sep 20, 2024 •
edited
Loading

Address #4203 (comment)

Side note: based on #4220 (comment) it seems that the current list of parameters is OK

@pellared pellared added area:api Cross language API specification issue spec:logs Related to the specification/logs directory labels Sep 20, 2024
@pellared pellared self-assigned this Sep 20, 2024
@pellared pellared marked this pull request as ready for review September 20, 2024 18:27
@pellared pellared requested review from a team as code owners September 20, 2024 18:27
@pellared pellared changed the title Do not allow extending Logger.Enabled parameters Do not requiring making Logger.Enabled parameters extensible Sep 20, 2024
@pellared pellared changed the title Do not requiring making Logger.Enabled parameters extensible Do not require making Logger.Enabled parameters extensible Sep 20, 2024
@pellared pellared changed the title Do not require making Logger.Enabled parameters extensible Do not require making Logger.Enabled parameters list extensible Sep 20, 2024
@pellared pellared changed the title Do not require making Logger.Enabled parameters list extensible Do not require extensible Logger.Enabled parameters list Sep 20, 2024
@pellared pellared mentioned this pull request Sep 20, 2024
Open
trask
trask reviewed Sep 26, 2024
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we remove similar language for metrics and traces also?

There are currently no required parameters for this API. Parameters can be added in the future, therefore, the API MUST be structured in a way for parameters to be added.

@trask
Copy link
Member

trask commented Sep 26, 2024 •
edited
Loading

I'd like to clarify that my approval is based on my understanding that it's always ok to add parameters to functions at the spec level

EDIT and I think it's fine for Go to take the "performant route" for them, and if parameters are added in the future they can introduce a second function (with a new name) which accepts the new set of parameters.

@reyang reyang merged commit d6cb895 into open-telemetry:main Oct 1, 2024
6 checks passed
@pellared pellared deleted the ref-log-enabled branch October 2, 2024 07:25
@jack-berg jack-berg mentioned this pull request Oct 2, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trask trask trask approved these changes

@dashpole dashpole dashpole approved these changes

@reyang reyang reyang approved these changes

@jack-berg jack-berg jack-berg approved these changes

Assignees

@pellared pellared

@reyang reyang

Labels
area:api Cross language API specification issue spec:logs Related to the specification/logs directory
Projects
Go: Logs (GA)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pellared @trask @dashpole @reyang @jack-berg