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

Check group id, metric name, and group name uniqueness #382

Merged
merged 13 commits into from
Nov 22, 2024

Conversation

lquerel
Copy link
Contributor

@lquerel lquerel commented Sep 20, 2024

This PR detects duplications in the following semconv concepts:

  • Group ID
  • Metric name
  • Group name (e.g., resource and event names)

Since there are existing duplications in the current registries (see the example below), we return warnings instead of errors. As usual, the --future flag must be used for future versions of the registry.

Example: https://github.com/search?q=repo%3Aopen-telemetry%2Fsemantic-conventions+path%3A%2F%5Emodel%5C%2F%2F+feature_flag&type=code

For now, the group name duplication detection is disabled.

Closes #306

@lquerel lquerel requested a review from a team as a code owner September 20, 2024 00:49
@lquerel lquerel self-assigned this Sep 20, 2024
@lquerel lquerel added the enhancement New feature or request label Sep 20, 2024
@jsuereth
Copy link
Contributor

jsuereth commented Nov 6, 2024

@lquerel I think this is unblocked now if you can update the branch

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 73.0%. Comparing base (cab3125) to head (13d5421).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/weaver_diff/src/lib.rs 81.3% 11 Missing ⚠️
crates/weaver_resolver/src/registry.rs 95.2% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #382     +/-   ##
=======================================
+ Coverage   72.5%   73.0%   +0.4%     
=======================================
  Files         49      49             
  Lines       3635    3711     +76     
=======================================
+ Hits        2639    2710     +71     
- Misses       996    1001      +5     

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


🚨 Try these New Features:

for (key, provenances) in keys {
if provenances.len() > 1 {
// Deduplicate the provenances.
let provenances: HashSet<String> = provenances.into_iter().collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You can use itertools to .unique this when collector instead of forcing a hashset.

I think performance wise it's the same, just easier to read.

@lquerel lquerel merged commit fe2ed21 into open-telemetry:main Nov 22, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Check group id, metric name, and event name uniqueness
2 participants