-
Notifications
You must be signed in to change notification settings - Fork 183
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
First stab at documenting the art of defining semantic conventions #1707
base: main
Are you sure you want to change the base?
First stab at documenting the art of defining semantic conventions #1707
Conversation
Copilot
AI
left a comment
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.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
docs/general/how-to-define-semantic-conventions.md:69
- The phrase 'are involved into instrumentation efforts' should be 'are involved in instrumentation efforts'.
are involved into instrumentation efforts, and are committed to be the point of contact for
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.
This is a great start!
|
||
- New conventions MUST have a group of codeowners. See [project management](https://github.com/open-telemetry/community/blob/main/project-management.md) for more details. | ||
<!-- TODO: add CI check for CODEOWNERS file (when a new area is added) --> | ||
- New conventions SHOULD be defined in YAML files. See [YAML Model for Semantic Conventions](/model/README.md) for the details. |
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.
With this SHOULD I assume your intention is that things should be added to yaml files where possible, but also it's OK to add conventions as just text in markdown files? For example how to record span names?
If this is right, maybe we can say something like this, to make it more clear?
- New conventions MUST be defined in YAML files. Conventions that cannot be recorded as part of the YAML model MAY be added in the corresponding markdown file (e.g., conventions for span name)
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 want to put MUST here, but there are plenty of cases where we can't. Span names is a good example, but we have other things we can't yet express: references to metrics, complex attribute types, resources that are expected to be associated with telemetry, etc.
I use SHOULD for new conventions and MUST as a prereq to stabilization.
I feel REQUIRING yaml for experimental stuff is not necessary, but also, it feels wrong to require something none of the conventions fully implement.
Maybe once the tooling covers 90% of scenarios we can revisit it and be more strict?
them if most of the following apply: | ||
|
||
- They provide a clear benefit to end users by enhancing telemetry. | ||
- You plan to use the attribute in spans, metrics, events, resources, or other telemetry signals. |
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.
- You plan to use the attribute in spans, metrics, events, resources, or other telemetry signals. | |
- There is a clear plan to use the attribute in spans, metrics, events, resources, or other telemetry signals. |
I'm never sure if we should avoid using "you" in conventions 🤔
|
||
- They provide a clear benefit to end users by enhancing telemetry. | ||
- You plan to use the attribute in spans, metrics, events, resources, or other telemetry signals. | ||
- The attribute will be utilized in instrumentations. |
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.
- The attribute will be utilized in instrumentations. | |
- The attribute will be utilized in instrumentations. |
If the above is true, isn't this already somewhat implicit? (that they are used in spans/metrics/logs etc?
I think what we have been trying to push when folks want to add new things, is that we'd like to see implementations of them, so maybe we can also say:
- There is a clear plan on how these attributes will be used by instrumentations
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.
If the above is true, isn't this already somewhat implicit? (that they are used in spans/metrics/logs etc?
I'd like to avoid someone defining conventions without planning to actually implement it. Changed the wording slightly to make it more clear. LMK what you think!
Also, some of the attributes are not really part of any specific telemetry signal. E.g. code
, thread
, enduser
, session
- it's something you stamp on any span/log/etc to enrich it. For those we'd probably not require referencing them on a span/metric/etc, but would ask about instrumentations and some plaintext description of how they are used.
There is a clear plan on how these attributes will be used by instrumentations
I like this, thanks!
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.
Minor grammar feedback
2afb348
to
6f90089
Compare
A lot of todos, which I'd like to tackle in follow up PRs (hopefully with the help of others)
Merge requirement checklist
[chore]