-
Notifications
You must be signed in to change notification settings - Fork 267
feat(semconv): update to v1.25 in a non-breaking way #1651
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
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. |
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 |
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 <[email protected]>
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
* 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.
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 <[email protected]> Co-authored-by: Kayla Reopelle <[email protected]>
* Layout/HeredocIndentation: Use 2 spaces for indentation in a heredoc. * Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. * Style/StringConcatenation: Prefer string interpolation to string concatenation. - ... except we don't want *interpolation*; we want to join pathname components with the correct separator for the operating system. So we'll use Pathname.join explicitly instead of Pathname's :+ operator so Rubocop doesn't think we're concatonating strings.
* Style/RedundantCurrentDirectoryInPath: Remove the redundant current directory path * Style/RedundantFileExtensionInRequire: Redundant .rb file extension detected.
5708734
to
e8357d8
Compare
Following up from Slack, here's a PR with my tweaks that bump the version to 1.36.0, which was the latest semconv version last week. The script worked great! I made a few small tweaks to account for formatting (rubocop) and the tests. Otherwise, this seems very easy to maintain as long as weaver remains stable. I'm comfortable approving this PR with those changes. I think we should also update the README to reflect this new structure, which I would be happy to do. What else do we need to consider before we merge this? What am I missing? |
Merged!
The questions we've asked ourselves in this PR that I don't know the answers to:
† And I think this as the primary gold-plater here. |
Guh. Linters, amirite? |
Ughhhh I forgot to run rubocop locally before I opened the PR. 🫠 At least the problems are only related to human-written code and not the autogenerated stuff! I'll add some code suggestions to fix things up. |
semantic_conventions/test/opentelemetry/semantic_conventions/attributes_test.rb
Show resolved
Hide resolved
Yup. +1 here. Let's leave it.
Also agree!
I think that's a good call. We can create an issue to take care of this later on if we feel strongly about it. |
Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Minitest/EmptyLineBeforeAssertionMethods: Add empty line before assertion. Co-authored-by: Kayla Reopelle <[email protected]>
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.
🎊
Whenever we want to look at README improvements, here's a draft: robbkidd#2 |
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?