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

[sdk] Allow / in metric instrument names #4882

Merged

Conversation

tombiddulph
Copy link
Contributor

@tombiddulph tombiddulph commented Sep 24, 2023

Fixes #4877
Design discussion issue #

Changes

Updates the regex in MeterProviderBuilderSdk.cs to allow metric instrument names to contain / characters. This is in response to the issue raised in #4887. Now, metric names like my_metric/environment/database can be used. Corresponding changes are also made to the tests and CHANGELOG.md.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes

Updates the regex in `MeterProviderBuilderSdk.cs` to allow metric instrument names to contain `/` characters. This is in response to the issue raised in [open-telemetry#4887](open-telemetry#4877). Now, metric names like `my_metric/environment/database` can be used. Corresponding changes are also made to the tests and CHANGELOG.md.
@tombiddulph tombiddulph requested a review from a team as a code owner September 24, 2023 15:57
tombiddulph and others added 2 commits September 25, 2023 17:05
Co-authored-by: Reiley Yang <reyang@microsoft.com>
@vishweshbankwar
Copy link
Member

@tombiddulph Could you please resolve the conflicts. Thanks!

@cijothomas
Copy link
Member

Thanks @tombiddulph !

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #4882 (19fdf67) into main (48b4a8c) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4882      +/-   ##
==========================================
- Coverage   83.15%   82.94%   -0.22%     
==========================================
  Files         294      294              
  Lines       12188    12188              
==========================================
- Hits        10135    10109      -26     
- Misses       2053     2079      +26     
Flag Coverage Δ
unittests 82.94% <100.00%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...lemetry/Metrics/Builder/MeterProviderBuilderSdk.cs 95.08% <100.00%> (ø)

... and 8 files with indirect coverage changes

@CodeBlanch CodeBlanch added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Sep 25, 2023
@CodeBlanch CodeBlanch changed the title Allow / in metric instrument names [sdk] Allow / in metric instrument names Sep 25, 2023
@CodeBlanch CodeBlanch merged commit a80d7ce into open-telemetry:main Sep 26, 2023
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "/" in instrument names
7 participants