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

Adopt attribute requirement levels in semantic conventions #2594

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Adopt attribute requirement levels in semantic conventions #2594

merged 2 commits into from
Jun 14, 2022

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jun 1, 2022

Updates existing conventions to use attribute requirement levels introduced in #2522

@lmolkova lmolkova requested review from a team as code owners June 1, 2022 05:04
@lmolkova lmolkova force-pushed the all-spec-requirement-levels branch from 9838950 to d1ff577 Compare June 1, 2022 05:08
@arminru arminru mentioned this pull request Jun 1, 2022
Closed
Oberon00
Oberon00 approved these changes Jun 1, 2022
View reviewed changes
semantic_conventions/trace/database.yaml Outdated Show resolved Hide resolved
@reyang reyang added the area:semantic-conventions Related to semantic conventions label Jun 1, 2022
@lmolkova lmolkova force-pushed the all-spec-requirement-levels branch 2 times, most recently from 7c6f70f to 23f8f02 Compare June 1, 2022 17:06
bogdandrutu
bogdandrutu reviewed Jun 1, 2022
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Reading this PR, just scares me how many things are "required". Do we need an audit of these, or we already checked that we really "require" this?

One last thing, in a separate PR: Can we document if changing "requirement_levels" is considered backwards compatible, and if yes between which levels?

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 2, 2022 •
edited
Loading

Reading this PR, just scares me how many things are "required". Do we need an audit of these, or we already checked that we really "require" this?

across all specs we only have 26 attributes that are required (some are duplicated in traces and metrics):

Most of required attributes look reasonable to me, I created issues where I believe we can relax the level:

We also have ~25 conditionally_required attributes, I reviewed them and they seem reasonable to me (mind the messaging rewrite and faas issue).

@lmolkova lmolkova force-pushed the all-spec-requirement-levels branch from c5f3098 to 42f9758 Compare June 2, 2022 17:38
@lmolkova lmolkova mentioned this pull request Jun 2, 2022
Closed
@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 2, 2022

One last thing, in a separate PR: Can we document if changing "requirement_levels" is considered backwards compatible, and if yes between which levels?

Great point, here it is #2601

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 3, 2022

@bogdandrutu let me know if you see #2600 and #2599 are preconditions without which this PR can't be merged.
From my side, I'd prefer to merge this one first, and then I'll send small follow-up PRs for the above issues.

@lmolkova lmolkova mentioned this pull request Jun 6, 2022
Closed
@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 7, 2022

@open-telemetry/specs-metrics-approvers can you please take a look?

bogdandrutu
bogdandrutu approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Discuss that @lmolkova will review "required" attributes in a different PR.

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 9, 2022

@jack-berg @lzchen @MrAlias @cijothomas @justinfoote @jkwatson can someone please review on behalf of @open-telemetry/specs-metrics-approvers ?

@jkwatson
Copy link
Contributor

jkwatson commented Jun 9, 2022

@jack-berg @lzchen @MrAlias @cijothomas @justinfoote @jkwatson can someone please review on behalf of @open-telemetry/specs-metrics-approvers ?

I need to be taken off that list.

jack-berg
jack-berg approved these changes Jun 9, 2022
View reviewed changes
semantic_conventions/resource/process.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/database.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/instrumentation/aws-sdk.yml Show resolved Hide resolved
@reyang
Copy link
Member

reyang commented Jun 10, 2022

@jack-berg @lzchen @MrAlias @cijothomas @justinfoote @jkwatson can someone please review on behalf of @open-telemetry/specs-metrics-approvers ?

I need to be taken off that list.

@jkwatson I've removed you from the list. THANK YOU for helping metrics API/SDK specs to the shape!

@lmolkova lmolkova force-pushed the all-spec-requirement-levels branch from 706542a to 4e773c0 Compare June 13, 2022 18:06
@lmolkova
Copy link
Contributor Author

@open-telemetry/technical-committee can someone please merge?

@lmolkova
parent bff2cc1
faf6875 
author Liudmila Molkova <limolkova@microsoft.com> 1652725143 -0700
committer Liudmila Molkova <limolkova@microsoft.com> 1655143447 -0700

parent bff2cc1
author Liudmila Molkova <limolkova@microsoft.com> 1652725143 -0700
committer Liudmila Molkova <limolkova@microsoft.com> 1655143419 -0700

parent bff2cc1
author Liudmila Molkova <limolkova@microsoft.com> 1652725143 -0700
committer Liudmila Molkova <limolkova@microsoft.com> 1655143334 -0700

Specify requirement levels in existing conventions
@lmolkova
lint
a909e22
@lmolkova lmolkova force-pushed the all-spec-requirement-levels branch from db613a7 to a909e22 Compare June 14, 2022 14:46
@reyang reyang merged commit 94c9c75 into open-telemetry:main Jun 14, 2022
@reyang reyang mentioned this pull request Jul 18, 2022
Merged
This was referenced Aug 21, 2022
Closed
Merged
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@lmolkova @beeme1mr
Adopt attribute requirement levels in semantic conventions (open-tele…
9592d82 
…metry#2594)
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@lmolkova @beeme1mr
Adopt attribute requirement levels in semantic conventions (open-tele…
22be553 
…metry#2594)
@joaopgrassi joaopgrassi mentioned this pull request Sep 6, 2022
Merged
This was referenced Oct 7, 2022
Closed
Closed
@srikanthccv srikanthccv mentioned this pull request Nov 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oberon00 Oberon00 Oberon00 approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@cijothomas cijothomas cijothomas approved these changes

@MrAlias MrAlias MrAlias approved these changes

@arminru arminru arminru approved these changes

@reyang reyang reyang approved these changes

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

Assignees

@tigrannajaryan tigrannajaryan

Labels
area:semantic-conventions Related to semantic conventions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@lmolkova @jkwatson @reyang @Oberon00 @bogdandrutu @cijothomas @MrAlias @arminru @jack-berg @tigrannajaryan