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

Upgrade instrumentation_library_metrics to scope_metrics #1507

Conversation

owent
Copy link
Member

@owent owent commented Jul 20, 2022

Signed-off-by: owentou [email protected]

Fixes #1492
Fixed #1506

Changes

  • Change the name of instrumentation_info_metric_data_ in ResourceMetrics and instrumentation_info_metrics in MetricCollector::Collect
  • PopulateResourceMetrics to scope_metrics in OTLP exporters
  • Rename InstrumentationInfoMetrics to ScopeMetrics
  • Rename instrumentation_library_ to scope_
  • Rename instrumentation_metrics to scope_metrics
  • Rename InstrumentationLibrary to InstrumentationScope
  • Rename GetInstrumentationLibrary to GetInstrumentationScope
  • Rename SetInstrumentationLibrary to SetInstrumentationScope
  • Rename printInstrumentationLibrary to printInstrumentationScope
  • Rename instrumentation_library_ to instrumentation_scope_
  • Rename instrumentation_library to instrumentation_library
  • Move instrumentationlibrary/instrumentation_library.h to instrumentationscope/instrumentation_scope.h
  • Add deprecated message for legacy GetInstrumentationLibrary and SetInstrumentationLibrary

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

@owent owent requested a review from a team July 20, 2022 05:03
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #1507 (ec2f2bb) into main (0015475) will increase coverage by 0.02%.
The diff coverage is 90.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1507      +/-   ##
==========================================
+ Coverage   84.61%   84.63%   +0.02%     
==========================================
  Files         155      155              
  Lines        4782     4781       -1     
==========================================
  Hits         4046     4046              
+ Misses        736      735       -1     
Impacted Files Coverage Δ
...de/opentelemetry/exporters/ostream/span_exporter.h 100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/metrics/meter.h 0.00% <0.00%> (ø)
...clude/opentelemetry/sdk/metrics/sync_instruments.h 100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/trace/recordable.h 100.00% <ø> (ø)
sdk/src/metrics/state/metric_collector.cc 50.00% <0.00%> (ø)
sdk/src/metrics/meter.cc 6.32% <83.34%> (+0.07%) ⬆️
sdk/include/opentelemetry/sdk/trace/span_data.h 98.67% <87.50%> (ø)
exporters/ostream/src/metric_exporter.cc 91.84% <100.00%> (ø)
exporters/ostream/src/span_exporter.cc 90.82% <100.00%> (ø)
exporters/ostream/test/ostream_metric_test.cc 100.00% <100.00%> (ø)
... and 8 more

@lalitb
Copy link
Member

lalitb commented Jul 21, 2022

Thanks for the PR. Looks good in general. One suggestion - do you think we should introduce opentelemetry::sdk::instrumentationScope::InstrumentationScope class and start using that for metrics (and probably also for logs). We can later deprecate "InstrumentationLibrary" once we remove it from traces. This way, we also add field attributes in InstrumentationScope as introduced here - open-telemetry/opentelemetry-specification#2579. Should be fine to do this in separate or current PR.

@owent
Copy link
Member Author

owent commented Jul 22, 2022

Thanks for the PR. Looks good in general. One suggestion - do you think we should introduce opentelemetry::sdk::instrumentationScope::InstrumentationScope class and start using that for metrics (and probably also for logs). We can later deprecate "InstrumentationLibrary" once we remove it from traces. This way, we also add field attributes in InstrumentationScope as introduced here - open-telemetry/opentelemetry-specification#2579. Should be fine to do this in separate or current PR.

Agree, do you think if we can rename InstrumentationLibrary to InstrumentationScope and add a alias with using InstrumentationLibrary = InstrumentationScope to keep compatibility for one or two versions? It's a break change to stable trace APIs if we just change the name.

@lalitb
Copy link
Member

lalitb commented Jul 22, 2022

Thanks for the PR. Looks good in general. One suggestion - do you think we should introduce opentelemetry::sdk::instrumentationScope::InstrumentationScope class and start using that for metrics (and probably also for logs). We can later deprecate "InstrumentationLibrary" once we remove it from traces. This way, we also add field attributes in InstrumentationScope as introduced here - open-telemetry/opentelemetry-specification#2579. Should be fine to do this in separate or current PR.

Agree, do you think if we can rename InstrumentationLibrary to InstrumentationScope and add a alias with using InstrumentationLibrary = InstrumentationScope to keep compatibility for one or two versions? It's a break change to stable trace APIs if we just change the name.

Yes, that's a good idea. I was actually thinking to keep both, but your suggestion seems better, as cleanup would be easy.

@owent owent force-pushed the Upgrade_instrumentation_library_metrics_to_scope_metrics branch from 1e83f0f to 2e6ad6f Compare July 22, 2022 08:42
@owent
Copy link
Member Author

owent commented Jul 22, 2022

Thanks for the PR. Looks good in general. One suggestion - do you think we should introduce opentelemetry::sdk::instrumentationScope::InstrumentationScope class and start using that for metrics (and probably also for logs). We can later deprecate "InstrumentationLibrary" once we remove it from traces. This way, we also add field attributes in InstrumentationScope as introduced here - open-telemetry/opentelemetry-specification#2579. Should be fine to do this in separate or current PR.

Agree, do you think if we can rename InstrumentationLibrary to InstrumentationScope and add a alias with using InstrumentationLibrary = InstrumentationScope to keep compatibility for one or two versions? It's a break change to stable trace APIs if we just change the name.

Yes, that's a good idea. I was actually thinking to keep both, but your suggestion seems better, as cleanup would be easy.

Done

@owent owent force-pushed the Upgrade_instrumentation_library_metrics_to_scope_metrics branch from 648269b to e94545f Compare July 26, 2022 02:44
@owent owent force-pushed the Upgrade_instrumentation_library_metrics_to_scope_metrics branch from e94545f to 6d2bacc Compare July 27, 2022 04:32
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM once all the comments are resolved.

@lalitb
Copy link
Member

lalitb commented Jul 28, 2022

This looks good to merge, but being bigger PR, we can wait for @esigo or @ThomsonTan to review it.

@owent
Copy link
Member Author

owent commented Jul 29, 2022

This looks good to merge, but being bigger PR, we can wait for @esigo or @ThomsonTan to review it.

Thanks

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 @owent for the PR :)

@lalitb
Copy link
Member

lalitb commented Jul 29, 2022

@owent - Need to rebase this to merge :)

@owent owent force-pushed the Upgrade_instrumentation_library_metrics_to_scope_metrics branch from 6d2bacc to 45bc9ea Compare July 30, 2022 08:05
@owent
Copy link
Member Author

owent commented Jul 30, 2022

@owent - Need to rebase this to merge :)

Thanks and done.

@owent owent mentioned this pull request Jul 30, 2022
3 tasks
@lalitb lalitb merged commit 59e7496 into open-telemetry:main Jul 30, 2022
@owent owent deleted the Upgrade_instrumentation_library_metrics_to_scope_metrics branch August 2, 2022 06:12
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
…lemetry#1507)

* Upgrade `instrumentation_library_metrics` to `scope_metrics`

Signed-off-by: owentou <[email protected]>

* Rename `InstrumentationLibrary` -> `InstrumentationScope`

Signed-off-by: owentou <[email protected]>

* Fix style

Signed-off-by: owentou <[email protected]>

* Fix compiling

Signed-off-by: owentou <[email protected]>

* Remove duplicated `ElasticSearchRecordable::SetInstrumentationLibrary`

Signed-off-by: owentou <[email protected]>

* Fix comments

Signed-off-by: owentou <[email protected]>

* Update the usage of `instrumentation_library` of prometheus exporter.

Signed-off-by: owentou <[email protected]>
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
…lemetry#1507)

* Upgrade `instrumentation_library_metrics` to `scope_metrics`

Signed-off-by: owentou <[email protected]>

* Rename `InstrumentationLibrary` -> `InstrumentationScope`

Signed-off-by: owentou <[email protected]>

* Fix style

Signed-off-by: owentou <[email protected]>

* Fix compiling

Signed-off-by: owentou <[email protected]>

* Remove duplicated `ElasticSearchRecordable::SetInstrumentationLibrary`

Signed-off-by: owentou <[email protected]>

* Fix comments

Signed-off-by: owentou <[email protected]>

* Update the usage of `instrumentation_library` of prometheus exporter.

Signed-off-by: owentou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants