Skip to content

feat: add unit to view instrument selection criteria#3647

Merged
pichlermarc merged 6 commits intoopen-telemetry:mainfrom
jlabatut:add-unit-to-view
Mar 6, 2023
Merged

feat: add unit to view instrument selection criteria#3647
pichlermarc merged 6 commits intoopen-telemetry:mainfrom
jlabatut:add-unit-to-view

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 March 1, 2023 17:24
@linux-foundation-easycla
Copy link

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

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
Loading

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

pichlermarc commented Mar 2, 2023

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 added 2 commits March 2, 2023 23:01
@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 🙂

dyladan and others added 2 commits March 3, 2023 08:58
@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