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.

Sign up for GitHub

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

Jump to bottom

feat: add unit to view instrument selection criteria #3647

Merged
merged 6 commits into from
Mar 6, 2023
Merged

feat: add unit to view instrument selection criteria #3647

merged 6 commits into from
Mar 6, 2023

Conversation

jlabatut
Copy link
Contributor

@jlabatut jlabatut commented Mar 1, 2023

Which problem is this PR solving?

Allow unit as an instrument selection criterion.

Fixes #3643

Short description of the changes

  • adding unitFilter to InstrumentSelector, as an ExactPredicate

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • npm test

@jlabatut jlabatut requested a review from a team as a code owner March 1, 2023 17:24
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 1, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jlabatut / name: Julian Labatut (ff95dd4)

@jlabatut jlabatut changed the title feat: add unit to view instrument selection criteria (#3647) feat: add unit to view instrument selection criteria Mar 1, 2023
@jlabatut jlabatut mentioned this pull request Mar 1, 2023
Closed
@dyladan dyladan added spec-feature This is a request to implement a new feature which is already specified by the OTel specification sdk:metrics Issues and PRs related to the Metrics SDK signal:metrics Issues and PRs related to general metrics signal labels Mar 1, 2023
@codecov
Copy link

codecov bot commented Mar 1, 2023 •
edited

Codecov Report

Merging #3647 (2e70eac) into main (ebc8575) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2e70eac differs from pull request most recent head 57fb894. Consider uploading reports for the commit 57fb894 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3647   +/-   ##
=======================================
  Coverage   93.63%   93.63%           
=======================================
  Files         275      275           
  Lines        8090     8095    +5     
  Branches     1679     1682    +3     
=======================================
+ Hits         7575     7580    +5     
  Misses        515      515
Impacted Files Coverage Δ
...ages/sdk-metrics/src/view/RegistrationConflicts.ts 97.43% <ø> (ø)
...ackages/sdk-metrics/src/view/InstrumentSelector.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/view/View.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/view/ViewRegistry.ts 100.00% <100.00%> (ø)

dyladan
dyladan reviewed Mar 1, 2023
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Implementation looks good but I'd like to see tests added for the functionality

@pichlermarc
Copy link
Member

I'd also echo @dyladan on the tests 🙂

One added test similar to should match view with instrument type in ViewRegistry.test.ts and one test in MeterProvider - addView changing the aggregation for instruments with one exact unit should be plenty to get this merged 🙂

Feel free to reach out here or on the CNCF Slack if you have any questions 🙂

@jlabatut
feat: add unit to view instrument selection criteria (open-telemetry#…
245cab2 
…3647)

Signed-off-by: Julian Labatut <julian.labatut@etu.umontpellier.fr>
@dyladan
Copy link
Member

dyladan commented Mar 2, 2023

Please do not modify history or force push on open PRs. It makes it difficult to follow changes made since the last review. A simple merge of main is enough. The PR is squashed when it is merged anyway.

@dyladan
Copy link
Member

dyladan commented Mar 2, 2023

This also needs a changelog entry

@jlabatut
fix: update CHANGELOG.md (open-telemetry#3647)
614cec3 
Signed-off-by: Julian Labatut <julian.labatut@etu.umontpellier.fr>
@jlabatut
test(sdk-metrics): add instrumentUnit tests (open-telemetry#3647)
17411dd 
Signed-off-by: Julian Labatut <julian.labatut@etu.umontpellier.fr>
@jlabatut jlabatut requested a review from dyladan March 2, 2023 22:06
pichlermarc
pichlermarc reviewed Mar 3, 2023
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests 🙂
One more change and this should be good to go 🙂

packages/sdk-metrics/test/MeterProvider.test.ts Outdated Show resolved Hide resolved
@jlabatut jlabatut requested review from pichlermarc and removed request for dyladan March 4, 2023 18:57
pichlermarc
pichlermarc approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good 🚀
thank you for your contribution. 🙂

@pichlermarc pichlermarc merged commit 3a2ca58 into open-telemetry:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyladan dyladan dyladan left review comments

@legendecas legendecas legendecas approved these changes

@pichlermarc pichlermarc pichlermarc approved these changes

Assignees
No one assigned
Labels
sdk:metrics Issues and PRs related to the Metrics SDK signal:metrics Issues and PRs related to general metrics signal spec-feature This is a request to implement a new feature which is already specified by the OTel specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add unit to view instrument selection criteria
4 participants
@jlabatut @pichlermarc @dyladan @legendecas