-
Notifications
You must be signed in to change notification settings - Fork 240
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
semconv 1.25: update in a non-breaking way #1651
base: main
Are you sure you want to change the base?
Conversation
semantic_conventions/lib/opentelemetry/semantic_candidates/attributes/aws.rb
Outdated
Show resolved
Hide resolved
Probably going to dial this back from semconv v1.26 to v1.25 because there's some name hijinks going on in |
7e2cdf2
to
64a9864
Compare
Guh. New markdown processor (redcarpet) for yard to use in doc generation doesn't support JRuby because native extensions.
|
Taking this out of draft. It may not be ready to merge, but it's ready for people to review and form opinions about. There's a lot going on here†, so walking the commits might help. I left notes about the whats and whys of each change in the commit messages. |
* pull from the new semconv repo * update jinja2 template for the new structure of convention data * stable semconv names will appear in OpenTelemetry::SemanticConventions while all semconv names (experimental, deprecated, stable, etc) appear in OpenTelemetry::SemanticCandidates - This maps to what other langs (Java, Python, JS) are putting into "Incubating" modules. We opted for "candidates" because OpenTelemetry::SemanticConventions::Incubating::... was getting too long and OpenTelemetry::SemanticIncubating didn't make much sense. * generate a module per root semconv namespace to make navigating docs more focused and to decrease the number of constants loaded when using more precise requires in downstream code * generates only attribute name constants and to an attributes subfolder to prepare for metric name constants to be generated to metrics subfolders, also for decreasing the scope of what gets loaded when downstream instrumentation requires these constants Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
Because converting to SCREAMING_SNAKE_CASE cannot distinguish between ... screaming.snake_case ... and ... screaming.snake.case So, like other languages, ignore the deprecated messaging.client_id in favor of its replacement and let backends deal with the changed attr name.
277 runs, 500 assertions, 26 failures, 0 errors, 0 skips Apparently there are some kinks to work out.
There's naming hijinks in the db.* namespace I would like to avoid in this first effort to bring semconv up-to-recent-date.
Explain the auto-generated situation.
The v1.10 names whose comments were updated in this commit were renamed or removed in version 1.11-1.25. Made note of their replacements or that their use should be dropped. Removed test for one-to-one matching of 1.10 constants to 1.25 "all" constants. That will never pass without updating the source semconv YAML to add back the missing names with dep notices. I believe our dep notes will suffice and we won't auto-generate Resource or Trace again.
* Move to a tmp/ dir and ignore that. * Update the git commands to be able to reuse one directory for the repo working copy, just update what's needed for a different tag between iterations. (Generally only an improvement for dev envs.) * Use sh() for shell commands so that tasks fail if their commands fail. * Appease Rubocop.
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
e998839
to
97daf90
Compare
Yard's default use of the rdoc markdown provider was choking on links containing codeblocks, for example: [`ENV`](link/to/somewhere). These style links are pretty popular in the text details provided with semconv attributes. Let's switch to kramdown which is a markdown parser and renderer that: * can handle inline code blocks inside a link anchor * is pure Ruby (no C extensions) so works with JRuby
97daf90
to
0446676
Compare
I wondering if some const_missing hijinks ought to be introduced to these Modules to protect downstream developers from semconv schema mistakes or mind-changes that remove attribute names from a version which would result in a missing constant that existed previously. If only to warn-log the missing constant once and return something that (1) prevents runtime code from blowing up and (2) gives some indication of unhappiness in the emitted telemetry that humans actually look at. |
Wanted to note while I'm looking at it - there's a chance we may go with Incubating in JS. If we do, it may be worth reconsidering "candidates" to help with consistency, and also to follow what seems to be guidance provided from semconv on artifact structure
So maybe OpenTelemetry::SemConvIncubating:: as an option? |
@JamieDanielson - I like the idea of mirroring the package name more closely and reducing So to match the artifact namespace more closely, it could also be: |
Yea. "Candidates" was probably never going to fly. This whole thing is about trying to be conventional. I don't hate shortening
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
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 looks good!
As we discussed in the comments, the plan is to update the constant prefixes to:
OpenTelemetry::SemConv
- stable constants
OpenTelemetry::SemConv::Incubating
- others
Once that's done, I think this will be ready to go. Adding the "request changes" label to make it easier to see.
* out with Candidates, we're using Incubating like most other SIGs * shorten the namespace to SemConv * do not provide an all-in-one rollup require for all namespaces; downstream users must directly require the namespaces that they use * attributes that are templates (prefixes, really, for the moment) are generated as lambdas with _LAMBDA appended to their name; users must call that lambda with a key for the template to return the attribute name string
With multiple template entries in weaver.yaml, code generation for stable and incubating can be performed in one invocation of the docker image. Rakefile targets trimmed down in light of that.
Pairing recap! @robb - Next steps:
module OpenTelemetry
module SemConv
module HTTP
end
end
end Questions:
Other stuff:
|
Each root namespace gets separate attributes and metrics files. Include examples of possible values for attributes. Include a usage example of the template attributes as lambdas. Fix links in doc comments referencing stable constants. Co-authored-by: Hannah Ramadan <hannahramadan@users.noreply.github.com> Co-authored-by: Kayla Reopelle <kaylareopelle@users.noreply.github.com>
Now that semconv won't have deprecated names removed without a major version bump, here's a go at updating semantic conventions to 1.25.
no
const_missing
hijinks! (like in 🚧 WIP: update semantic conventions in a non-breaking way #1567)pulls from the new semconv repo
updates jinja2 template for the new structure of convention data
stable semconv names will appear in OpenTelemetry::SemanticConventions
while all semconv names (experimental, deprecated, stable, etc) appear
in OpenTelemetry::SemanticCandidates
"Incubating" modules. We opted for "candidates" because
OpenTelemetry::SemanticConventions::Incubating::... was getting
too long and OpenTelemetry::SemanticIncubating didn't make much
sense.
generates a module per root semconv namespace to make navigating docs
more focused and to decrease the number of constants loaded when
using more precise requires in downstream code
generates only attribute name constants and to an attributes subfolder
to prepare for metric name constants to be generated to metrics
subfolders, also for decreasing the scope of what gets loaded when
downstream instrumentation requires these constants
Some TODOs
A few of these appear in the code in this PR, too.
test(s) to make sure the trace and resource constants are present in SemanticCandidatesSemanticConventions
(stable) constants are all still present in theSemanticCandidates
constantsSemanticConventions::(Resource|Trace)
constants with pointers to their replacementsFirstcap
orPascalCase
the module names in the jinja2 template with exceptions for things like AspNetCore, HTTP, etc?SCREAMING
module names for simplicity of up-keep.Turn off testing constants exist on JRuby?