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.

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Jul 10, 2024

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

    • 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.
  • 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.

  • usage examples. How weird is it to explicitly require the nested modules?
  • test(s) to make sure the trace and resource constants are present in SemanticCandidates
    • See dc42bf5. I did the best I could with a test to find which existing v1.10 constants had no match in the entirety of 1.25. I used that list of missing consts to update their deprecation notices accordingly.
  • test(s) to make sure the SemanticConventions (stable) constants are all still present in the SemanticCandidates constants
  • add deprecation notices to all the SemanticConventions::(Resource|Trace) constants with pointers to their replacements
  • figure out how to Firstcap or PascalCase the module names in the jinja2 template with exceptions for things like AspNetCore, HTTP, etc?
    • The module name will become a part of the constants interface, so the fancier we make the code generation logic, the more likely something breaks and we release a version where module names have a new backwards-breaking shape.
    • 👎🏻 I vote we don't do this and have non-idiomatic SCREAMING module names for simplicity of up-keep.
  • Do something about JRuby
    • Turn off testing constants exist on JRuby?
    • Find a markdown parser/renderer (1) supported by Yard, (2) supports inline-code-blocks-inside-links in markdown content, and (3) works on JRuby?
      • Found kramdown. Using it instead of redcarpet.

@robbkidd robbkidd self-assigned this Jul 10, 2024
@robbkidd robbkidd changed the title 🚧 WIP: update semantic conventions in a non-breaking way (take 2) 🚧 semconv 1.26: update in a non-breaking way (take 2) Jul 10, 2024
@robbkidd
Copy link
Member Author

Probably going to dial this back from semconv v1.26 to v1.25 because there's some name hijinks going on in db.* in 1.26. At least for this initial cut at catching Ruby's semconv library up to recent.

@robbkidd
Copy link
Member Author

Guh. New markdown processor (redcarpet) for yard to use in doc generation doesn't support JRuby because native extensions.

An error occurred while installing redcarpet (3.6.0), and Bundler cannot
continue.

In Gemfile:
  redcarpet

@robbkidd robbkidd changed the title 🚧 semconv 1.26: update in a non-breaking way (take 2) 🚧 semconv 1.25: update in a non-breaking way (take 2) Jul 12, 2024
@robbkidd robbkidd changed the title 🚧 semconv 1.25: update in a non-breaking way (take 2) semconv 1.25: update in a non-breaking way (take 2) Jul 12, 2024
@robbkidd robbkidd marked this pull request as ready for review July 12, 2024 16:56
@robbkidd
Copy link
Member Author

robbkidd commented Jul 12, 2024

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.

† 😅
Screenshot 2024-07-12 at 1 02 27 PM

@robbkidd robbkidd changed the title semconv 1.25: update in a non-breaking way (take 2) semconv 1.25: update in a non-breaking way Jul 12, 2024
robbkidd and others added 7 commits July 19, 2024 14:22
* 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.
robbkidd and others added 3 commits July 19, 2024 14:23
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>
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
@robbkidd
Copy link
Member Author

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.

@JamieDanielson
Copy link
Member

  • 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.

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

  • Artifact name:
    • opentelemetry-semconv - stable conventions
    • opentelemetry-semconv-incubating - (if applicable) the preview artifact containing all (stable and experimental) conventions
  • Namespace: opentelemetry.semconv and opentelemetry.semconv.incubating

So maybe OpenTelemetry::SemConvIncubating:: as an option?

@kaylareopelle
Copy link
Contributor

@JamieDanielson - I like the idea of mirroring the package name more closely and reducing SemanticConventions to SemConv. That'd give our fingers a bit of a break.

So to match the artifact namespace more closely, it could also be:
OpenTelemetry::SemConv - stable constants
OpenTelemetry::SemConv::Incubating - others

@robbkidd
Copy link
Member Author

robbkidd commented Aug 1, 2024

Yea. "Candidates" was probably never going to fly. This whole thing is about trying to be conventional.

I don't hate shortening SemanticConventions to SemConv, especially if we follow the guidance below and generate OpenTelemetry::SemConv::Incubating::Attributes::CLIENT::CLIENT_SOMETHING.

  • Attributes, metrics, and other convention definitions should be grouped by the convention type and the root namespace. See the example below:
├── SchemaUrls.code
├── attributes
│   ├── ClientAttributes.code
│   ├── HttpAttributes.code
│   └── ...
├── metrics
│   ├── HttpMetrics.code
│   └── ...
└── events
    └── ...

Copy link
Contributor

github-actions bot commented Sep 1, 2024

👋 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 keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label Sep 1, 2024
@kaylareopelle kaylareopelle added keep and removed stale labels Sep 3, 2024
Copy link
Contributor

@kaylareopelle kaylareopelle left a 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.
@kaylareopelle
Copy link
Contributor

Pairing recap!

@robb - Next steps:

  • Add some post-processing using Rake/Ruby to generate a rollup file for each namespace to require the attributes.rb and metrics.rb files
    • Use Ruby file tools to look at all the new directories that were created
    • Produce new files with those names at the same level as the directory (ex. semconv/http is the existing directory and semconv/http.rb will be the generated the rollup file)
    • Within the rollup file, require the attributes.rb and metrics.rb files
      • (maybe an ls on the root namespace directory may help verify the presence of an attributes.rb and metrics.rb within each namespace?)
    • Maybe add the empty module definition for the namespace in the rollup file too:
module OpenTelemetry
  module SemConv
    module HTTP
    end
  end
end

Questions:

  • Do we want some sort of identifier in the name of the constant to distinguish whether the constant is an attribute or a metric?
    • We have a label for templates (suffix *_LAMBDA)
    • The constant would look something like: OpenTelemetry::SemConv::HTTP::HTTP_SERVER_REQUEST_DURATION for a metric and OpenTelemetry::SemConv::HTTP::HTTP_STATUS_CODE for an attribute,
    • To match the file path for module name nesting: OpenTelemetry::SemConv::HTTP::Attribute::HTTP_STATUS_CODE
    • To match the augment the constant name: OpenTelemetry::SemConv::HTTP::HTTP_STATUS_CODE_ATTR

Other stuff:

  • Check to see if "Now available in the stable namespace..." comment in the incubating code is hyperlinkable to the stable constant
  • Can you put comments within a Heredoc? (docker command)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants