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

Attribute requirement levels #92

Merged
merged 16 commits into from
May 31, 2022
Merged

Attribute requirement levels #92

merged 16 commits into from
May 31, 2022

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Apr 26, 2022 •
edited
Loading

Tooling part of open-telemetry/opentelemetry-specification#2469 open-telemetry/opentelemetry-specification#2522

Breaks down attribute requiredness info more levels:

  • Required
  • Conditional (TODO, we need a better name): required under a specific condition (e.g. when available)
  • Recommended (optional)
  • Opt-in: optional and need to be explicitly enabled

@lmolkova lmolkova requested a review from a team April 26, 2022 04:13
@lmolkova
Copy link
Contributor Author

@Oberon00 @arminru please review along with the new terminology I propose in open-telemetry/opentelemetry-specification#2469

specifically here:

https://github.com/open-telemetry/opentelemetry-specification/blob/454f77b8b5e8b36e7335ae0bd3c7216aa59b0886/specification/common/attribute-requirement-level.md

@Oberon00
Copy link
Member

Oberon00 commented May 3, 2022

Hi! I think it would be great if the spec PR could be split into one that defines the new requirement levels and the actual HTTP-related changes.
That way, the requirement-level changes could be independently reviewed & merged, and we would only need to review the implementation & YAML syntax in this PR, not the actual definitions.

I think the build-tool repo won't attract many reviewers, and a PR (title) specifically for HTTP may give you different reviewers from the reviewers you would get on a PR that has a title indicating the more general, wider-reaching changes of introducing these new requirement levels.

@lmolkova lmolkova mentioned this pull request May 3, 2022
Merged
@lmolkova
Copy link
Contributor Author

lmolkova commented May 3, 2022

@Oberon00 thanks for the suggestion! Done in open-telemetry/opentelemetry-specification#2522

@lmolkova lmolkova mentioned this pull request May 16, 2022
Closed
@lmolkova
Copy link
Contributor Author

@Oberon00 @arminru

Would appreciate your review on this one.

Requirement level definitions open-telemetry/opentelemetry-specification#2522 got merged

open-telemetry/opentelemetry-specification#2542 shows what it'll mean for current semantic conventions.

@lmolkova
Copy link
Contributor Author

@thisthat @open-telemetry/specs-approvers can you please review?

@bogdandrutu
Copy link
Member

@lmolkova lots of maintainers/contributors are at Kubecon EU this week. Unfortunately my python skills are close to 0.

Oberon00
Oberon00 reviewed May 18, 2022
View reviewed changes
semantic-conventions/CHANGELOG.md Outdated Show resolved Hide resolved
semantic-conventions/semconv.schema.json Outdated Show resolved Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_attribute.py Outdated Show resolved Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_attribute.py Show resolved Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py Outdated Show resolved Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/code.py Outdated Show resolved Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/code.py Outdated Show resolved Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py Outdated Show resolved Hide resolved
semantic-conventions/src/tests/semconv/model/test_error_detection.py Outdated Show resolved Hide resolved
semantic-conventions/syntax.md Outdated Show resolved Hide resolved
@Oberon00 Oberon00 added semconv Related to the semantic convention generator. semconv/md Related specifically to the markdown output of the semantic convention generator semconv/model Related to the data model or YAML format of the semantic convention generator labels May 18, 2022
@lmolkova
Copy link
Contributor Author

@Oberon00 thank you! I believe I addressed your comments

@lmolkova
Copy link
Contributor Author

@thisthat can you please take a look?

@lmolkova
Copy link
Contributor Author

@Oberon00 if you're happy with this change, could you please merge it?
Also, what would it take to release so I can start updating specs to use requirement-levels?

@Oberon00
Copy link
Member

I'd be fine with merging, but don't have permission to do so. I think only @open-telemetry/technical-committee has maintainer access to this repository.

@lmolkova
Copy link
Contributor Author

@bogdandrutu or @jmacd can you please merge?

arminru
arminru approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Thank you, @lmolkova!

@arminru arminru merged commit 32fe017 into open-telemetry:main May 31, 2022
@joaopgrassi joaopgrassi mentioned this pull request Jul 8, 2022
Merged
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

@jmacd jmacd jmacd approved these changes

@arminru arminru arminru approved these changes

@thisthat thisthat Awaiting requested review from thisthat

Assignees
No one assigned
Labels
semconv/md Related specifically to the markdown output of the semantic convention generator semconv/model Related to the data model or YAML format of the semantic convention generator semconv Related to the semantic convention generator.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lmolkova @Oberon00 @bogdandrutu @jmacd @arminru