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

Make SDK Tracer/Meter/Logger Creation more normative #3529

Merged
merged 15 commits into from
Jun 26, 2023
Merged

Make SDK Tracer/Meter/Logger Creation more normative #3529

merged 15 commits into from
Jun 26, 2023

Conversation

pellared
Copy link
Member

@pellared pellared commented Jun 2, 2023 •
edited
Loading

Changes

  • "New Tracer/Meter/Logger instances are always created through a TracerProvicer/MeterProvider/LoggerProvider" is not normative. It is not desired in all languages (see Remove No-Op instrument and Meter creation restrictions #3322). I decided to change the wording to make it is a SHOULD. Additionally, "SHOULD" makes it as a recommendation to make sure we do not break any existing implementation.
  • Remove the (optional) and parameter names and simply say that the SDK MUST implement the API. This is done in order to minimize the confusion on what is "optional". It also makes the specification more normative. This is not breaking the stability requirements - this is just "fixing the spec".

@pellared pellared marked this pull request as ready for review June 2, 2023 10:27
@pellared pellared requested review from a team June 2, 2023 10:27
@arminru arminru added spec:metrics Related to the specification/metrics directory area:sdk Related to the SDK labels Jun 2, 2023
reyang
reyang requested changes Jun 2, 2023
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

For small changes, I think the high order bit is to keep consistency across signals https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#tracer-creation.

@pellared pellared marked this pull request as draft June 5, 2023 11:26
@pellared pellared changed the title Make SDK Meter Creation more normative [WIP] Make SDK Meter Creation more normative Jun 5, 2023
@pellared pellared changed the title [WIP] Make SDK Meter Creation more normative Make SDK Tracer/Meter/Logger Creation more normative Jun 5, 2023
@pellared pellared marked this pull request as ready for review June 5, 2023 11:44
@pellared pellared requested review from a team, reyang and cijothomas June 5, 2023 11:44
@pellared
Copy link
Member Author

pellared commented Jun 5, 2023

For small changes, I think the high order bit is to keep consistency across signals https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#tracer-creation.

Done

tigrannajaryan
tigrannajaryan requested changes Jun 5, 2023
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

These are changes to Stable part of the spec. For every change I believe we need to see an explanation about how this is allowed and is not breaking out stability guarantees.

@pellared
Copy link
Member Author

pellared commented Jun 6, 2023

These are changes to Stable part of the spec. For every change I believe we need to see an explanation about how this is allowed and is not breaking out stability guarantees.

I updated the description.

@pellared
Copy link
Member Author

pellared commented Jun 8, 2023

@tigrannajaryan @jack-berg Have I addressed/answered your comments?

tigrannajaryan
tigrannajaryan reviewed Jun 8, 2023
View reviewed changes
specification/logs/sdk.md Outdated Show resolved Hide resolved
specification/logs/sdk.md Outdated Show resolved Hide resolved
jack-berg
jack-berg reviewed Jun 8, 2023
View reviewed changes
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Can you add a changelog entry?

pellared and others added 2 commits June 12, 2023 09:00
@pellared @tigrannajaryan
Apply suggestions from code review
65201df 
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@pellared
Update CHANGELOG.md
56b2bef
@pellared
Copy link
Member Author

Can you add a changelog entry?

Done 56b2bef

@yurishkuro
Copy link
Member

Moreover, I am not sure if possible to accomplish in all languages.

How come? Examples where it's not possible?

@pellared
Copy link
Member Author

Moreover, I am not sure if possible to accomplish in all languages.

How come? Examples where it's not possible?

Changed to

It is not desired in all languages (see #3322).

Which is more clear and explicit

@pellared pellared requested a review from yurishkuro June 26, 2023 07:28
@reyang reyang merged commit d035298 into open-telemetry:main Jun 26, 2023
6 checks passed
@pellared pellared deleted the patch-5 branch June 26, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@cijothomas cijothomas cijothomas approved these changes

@reyang reyang reyang approved these changes

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

Assignees

@arminru arminru

Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pellared @yurishkuro @tigrannajaryan @cijothomas @reyang @jack-berg @arminru