-
Notifications
You must be signed in to change notification settings - Fork 512
[Metrics SDK] Add unit to Instrument selection criteria #2236
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2236 +/- ##
==========================================
+ Coverage 87.31% 87.34% +0.03%
==========================================
Files 169 169
Lines 4900 4902 +2
==========================================
+ Hits 4278 4281 +3
+ Misses 622 621 -1
|
marcalff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Please add a CHANGELOG entry, similar to this:
Breaking changes:
* [SDK] MeterProvider should own MeterContext, not share it
[#2218](https://github.com/open-telemetry/opentelemetry-cpp/pull/2218)
* The `MeterProvider` constructor now takes a `unique_ptr` on
`MeterContext`, instead of a `shared_ptr`.
* Please adjust SDK configuration code accordingly.
Agreed, looks better this way. |
esigo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks :)
sdk/include/opentelemetry/sdk/metrics/view/instrument_selector.h
Outdated
Show resolved
Hide resolved
|
Please add a CHANGELOG entry. |
Co-authored-by: Ehsan Saei <[email protected]>
…pp into add-unit-criteria
Done. Thanks. |
Fixes #1995
This is breaking change for View/ViewFactory API:
Old:
View(name, desc, agg_type, agg_config, attribute_processor)New:
View(name, desc, unit, agg_type, agg_config, attribute_processor)Old:
ViewFactory::Create(name, desc, agg_type, agg_config, attribute_processor)New:
ViewFactory::Create(name, desc, unit, agg_type, agg_config, attribute_processor)Although the addition of the
unitas an optional last argument can prevent any breaking changes, I just felt it would be more logical to include it alongside thenameanddescarguments.Changes
Add unit to instrument selection criteria.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes