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

Initial attempt at adding metric semconvs. #1918

Merged
merged 15 commits into from
Jul 12, 2024

Conversation

tbrockman
Copy link
Contributor

Fixes # TBD
Design discussion issue (if applicable) #
Slack context # https://cloud-native.slack.com/archives/C03GDP0H023/p1720453950905069

Changes

  • Creates a new semantic_metrics.rs.j2 Jinja template for producing metric.rs, which contains Metric semantic conventions.
  • Stores all attributes const &str in attributes.rs file
  • Other files (resource.rs, trace.rs) re-export attributes from attribute.rs to preserve backwards compatibility (though it might be desirable to break compatibility here as it seems resource.rs and trace.rs both contain duplicated attributes from attribute_group)

Generated docstring VSCode examples:
image
image

As mentioned in Slack, it seems like the code generation will be migrated to weaver, but I'd already wrote this and perhaps this could be useful in the interim.

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)

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.0%. Comparing base (621a5a9) to head (1e128ce).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1918   +/-   ##
=====================================
  Coverage   75.0%   75.0%           
=====================================
  Files        122     122           
  Lines      20290   20290           
=====================================
  Hits       15221   15221           
  Misses      5069    5069           

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

@cijothomas
Copy link
Member

to preserve backwards compatibility

The crate is pre-release (with no planned stable release date as of today), so don't worry too much about back compatibility. Feel free to do the correct thing, without feeling burdened by the need of back-compatbility for this crate.

@tbrockman tbrockman marked this pull request as ready for review July 10, 2024 13:47
@tbrockman tbrockman requested a review from a team as a code owner July 10, 2024 13:47
@tbrockman
Copy link
Contributor Author

to preserve backwards compatibility

The crate is pre-release (with no planned stable release date as of today), so don't worry too much about back compatibility. Feel free to do the correct thing, without feeling burdened by the need of back-compatbility for this crate.

Oh awesome, I wasn't really aware of the current release status, thanks for the heads up!

Changes made to remove those attributes from trace.rs and resource.rs, making them available through attribute.rs.

@tbrockman
Copy link
Contributor Author

Current CI failures are unrelated:

error: package `cc v1.1.0` cannot be built because it requires rustc 1.67 or newer, while the currently active rustc version is 1.65.0
Either upgrade to rustc 1.67 or newer, or use
cargo update -p cc@1.1.0 --precise ver
where `ver` is the latest version of `cc` supporting rustc 1.65.0
Error: Process completed with exit code 1.

Looks like #1924 might fix this, will update when that's merged.

@aumetra
Copy link
Contributor

aumetra commented Jul 10, 2024

Looks like #1924 might fix this, will update when that's merged.

Yep, my PR adds an entry to the patch_dependencies.sh script to downgrade the crate to the last version that fits the semver requirements (namely cc v1.0.105)

@lalitb
Copy link
Member

lalitb commented Jul 10, 2024

@tbrockman Can you also update changelog for the support of metrics semconvs, along with the breaking change for consolidation of attributes.

@tbrockman
Copy link
Contributor Author

@tbrockman Can you also update changelog for the support of metrics semconvs, along with the breaking change for consolidation of attributes.

Can do.

@tbrockman
Copy link
Contributor Author

@lalitb believe all related Cargo.toml's and CHANGELOG.md's should be up-to-date, let me know if I've missed anything.

@lalitb
Copy link
Member

lalitb commented Jul 10, 2024

@lalitb believe all related Cargo.toml's and CHANGELOG.md's should be up-to-date, let me know if I've missed anything.

@tbrockman Just few nit comments, should be good to go otherwise. Thanks for the contribution.

opentelemetry-semantic-conventions/Cargo.toml Outdated Show resolved Hide resolved
opentelemetry-zipkin/Cargo.toml Outdated Show resolved Hide resolved
opentelemetry-zipkin/CHANGELOG.md Outdated Show resolved Hide resolved
opentelemetry-zipkin/Cargo.toml Outdated Show resolved Hide resolved
@tbrockman
Copy link
Contributor Author

@lalitb makes sense, I wasn't entirely sure what the release process entailed. Happy to contribute, thanks for making the process quick and painless 😬

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.

Thanks @tbrockman . One nit comment.

@lalitb lalitb merged commit 597327f into open-telemetry:main Jul 12, 2024
25 checks passed
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.

None yet

4 participants