Skip to content

Conversation

@lalitb
Copy link
Member

@lalitb lalitb commented Jul 14, 2023

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 unit as an optional last argument can prevent any breaking changes, I just felt it would be more logical to include it alongside the name and desc arguments.

Changes

Add unit to instrument selection criteria.
For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team July 14, 2023 05:18
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #2236 (b308f07) into main (038e65d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/metrics/view/view.h 100.00% <ø> (ø)
...entelemetry/sdk/metrics/view/instrument_selector.h 100.00% <100.00%> (ø)
...ude/opentelemetry/sdk/metrics/view/view_registry.h 94.12% <100.00%> (+0.18%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@marcalff marcalff left a 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.

@marcalff
Copy link
Member

Although the addition of the unit as an optional last argument can prevent any breaking changes, I just felt it would be more logical to include it alongside the name and desc arguments.

Agreed, looks better this way.

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks :)

@esigo esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Jul 19, 2023
@marcalff
Copy link
Member

@lalitb

Please add a CHANGELOG entry.

@lalitb
Copy link
Member Author

lalitb commented Jul 20, 2023

@lalitb

Please add a CHANGELOG entry.

Done. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Metrics SDK] Add unit to Instrument selection criteria

3 participants