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

Use weaver for semantic convention codegen #2098

Merged
merged 29 commits into from
Sep 26, 2024
Merged

Conversation

lquerel
Copy link
Contributor

@lquerel lquerel commented Sep 10, 2024

Changes

This PR marks the first step in the migration to Weaver for semconv code generation. The following changes have been made:

  • Utilized Weaver’s new capabilities to simplify code generation as much as possible.
  • Moved to SemConv v1.27.0.
  • Added a blank line between each declaration for improved readability.
  • Attribute notes have been included in comments.
  • String examples are now represented using double quotes.
  • Boolean examples are represented with true or false, instead of 'True' or 'False'.
  • Number examples are represented without single quotes.
  • Experimental attributes are now gated behind the semconv_experimental feature.
  • Deprecated attributes include a note explaining the reason for deprecation.
  • Sorted metric attributes by name.

Note: This PR represents the first step of the plan described here.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@@ -20,54 +20,24 @@ git fetch origin "v$SPEC_VERSION"
git reset --hard FETCH_HEAD
cd "$CRATE_DIR"

docker run --rm \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only need to call Weaver once, as the weaver.yaml file specifies what needs to be generated.

@lquerel lquerel changed the title [WIP] Use weaver for semantic convention codegen Use weaver for semantic convention codegen Sep 11, 2024
@lquerel lquerel marked this pull request as ready for review September 11, 2024 21:26
@lquerel lquerel requested a review from a team September 11, 2024 21:26
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Per my understanding this PR only changes the generation tool we use. The end results are still a list of static string right?

@lquerel
Copy link
Contributor Author

lquerel commented Sep 11, 2024

Per my understanding this PR only changes the generation tool we use. The end results are still a list of static string right?

@TommyCpp @cijothomas

Yes, the initial intent was to minimize differences with the existing code generation first and then propose a more advanced integration that removes static strings entirely. However, if the community prefers to move directly to a step where static strings are replaced, such as for attribute declarations like:

pub const HTTP_REQUEST_METHOD: crate::attributes::AttributeKey<HttpRequestMethod> = crate::attributes::AttributeKey::new("http.request.method");

I am happy to update this PR accordingly. This would be straightforward since I already have a proof of concept (POC) for the step 2 described in #2100. Even if we don’t move directly to step 2, which is a fully type-safe API for Rust semconv, we could create an intermediate step (let’s call it step 1.5) where static strings are no longer visible to users or the compiler. I’d appreciate more feedback on this approach.

opentelemetry-semantic-conventions/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -1,60 +1,79 @@
// DO NOT EDIT, this is an auto-generated file
//
// If you want to update the file:
// - Edit the template at scripts/templates/semantic_attributes.rs.j2
// - Edit the template at scripts/templates/registry/rust/attributes.rs.j2
Copy link
Member

@lalitb lalitb Sep 12, 2024

Choose a reason for hiding this comment

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

The attributes.rs file currently defines semconv constants, which are then re-exported in various other modules (ie trace.rs, metrics.rs, resource.rs). If there aren't any (or too many) shared definitions used across multiple modules, would it make sense to move these constants to their respective modules where they actually belong? The attributes.rs can grow over time as we add more definitions, and add the semconv for logs later, could be difficult to manage even though auto-generated.

I know this has been the existing behavior, but then right time to decide any such change.

Copy link
Member

Choose a reason for hiding this comment

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

Just to add - this is not a blocker for the PR, something to discuss and plan for subsequent if all agree.

@lalitb
Copy link
Member

lalitb commented Sep 12, 2024

pub const HTTP_REQUEST_METHOD: crate::attributes::AttributeKey<HttpRequestMethod> = crate::attributes::AttributeKey::new("http.request.method");

@lquerel This looks awesome - in this case it will ensure that HTTP_REQUEST_METHOD will always have valid HTTP verbs. Just thinking in terms of the performance - we will possibly encapsulate these verbs in enum, and then encapsulate the '&static str for http.request.method in a structure AttributeKey. Do you see any performance overhead for (specifically) using them in hot path (as comparison to the existing approach) ?

@lquerel lquerel requested a review from a team as a code owner September 18, 2024 18:44
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.0%. Comparing base (0f15d21) to head (9cb825b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2098   +/-   ##
=====================================
  Coverage   79.0%   79.0%           
=====================================
  Files        121     121           
  Lines      20933   20933           
=====================================
  Hits       16549   16549           
  Misses      4384    4384           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lquerel
Copy link
Contributor Author

lquerel commented Sep 18, 2024

@lalitb I’ve fixed most of the issues, but there are still a few problems related to unescaped markdown links in the semconv registry. cargo doc rejects markdown text like blabla [0,n] blabla. Instead of finding a workaround in the Rust template, I plan to add a new configuration for the comment filter to automatically escape invalid links. See: Issue #374. This should be part of the next release.

@lalitb
Copy link
Member

lalitb commented Sep 20, 2024

@lalitb I’ve fixed most of the issues, but there are still a few problems related to unescaped markdown links in the semconv registry. cargo doc rejects markdown text like blabla [0,n] blabla. Instead of finding a workaround in the Rust template, I plan to add a new configuration for the comment filter to automatically escape invalid links. See: Issue #374. This should be part of the next release.

Thanks @lquerel. It seems if the CI is breaking, we need to wait for the next release. When is it planned?

@lquerel
Copy link
Contributor Author

lquerel commented Sep 20, 2024

@lalitb We are targeting a new release at the beginning of next week.

@lquerel
Copy link
Contributor Author

lquerel commented Sep 21, 2024

@lalitb @cijothomas Weaver release v0.10.0 fixed the issue with non-escaped invalid links present in the semconv registry that were causing problems with cargo doc. This PR is now ready for final review.

@lquerel lquerel requested a review from lalitb September 24, 2024 14:53
"${SED[@]}" "s/\(opentelemetry.io\/schemas\/\)[^\"]*\"/\1$SPEC_VERSION\"/" scripts/templates/registry/rust/weaver.yaml

docker run --rm \
--mount type=bind,source=$CRATE_DIR/semantic-conventions/model,target=/home/weaver/source,readonly \
Copy link
Contributor

Choose a reason for hiding this comment

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

For another review: This binding method seems to be incompatible with SELinux systems.
Is there a particular advantage ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are advantages. For context, I’ve copied the relevant change log entry below.

(#322, #312,
#319, #318,
#312, #304
by @jsuereth ) Many improvements have been made to the creation of the Weaver Docker image,
which is now scoring an A on the Scout Docker image score.

  • Add Weaver docker image to dependabot tracking,
  • Add build attestations,
  • Stop using root user the docker image,
  • Use official docker action to build docker image,
    Update docker to use release build.

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for getting start with this

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM to start with. Thanks

@lquerel
Copy link
Contributor Author

lquerel commented Sep 26, 2024

@lalitb @TommyCpp Do you know why the merge into the main is blocked?

@lalitb lalitb merged commit 161929d into open-telemetry:main Sep 26, 2024
24 checks passed
cijothomas pushed a commit to cijothomas/opentelemetry-rust that referenced this pull request Oct 4, 2024
Co-authored-by: Zhongyang Wu <zhongyang.wu@outlook.com>
Co-authored-by: Lalit Kumar Bhasin <labhas@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants