refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope by pichlermarc · Pull Request #2959 · open-telemetry/opentelemetry-js

pichlermarc · 2022-05-10T14:32:01Z

Which problem is this PR solving?

The spec renamed InstrumentationLibrary to InstrumentationScope. This PR renames InstrumentationLibrary to InstrumentationScope to match the spec (Metrics ONLY)

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)
- breaking change for experimental packages

How Has This Been Tested?

Unit Tests

Checklist:

Followed the style guidelines of this project
Unit tests have been ~~added~~ updated
Documentation has been updated

…entationLibrary.

codecov · 2022-05-10T14:37:32Z

Codecov Report

Merging #2959 (f37bc81) into main (4c99541) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2959   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files         183      183           
  Lines        5921     5921           
  Branches     1257     1257           
=======================================
  Hits         5494     5494           
  Misses        427      427

Impacted Files	Coverage Δ
...elemetry-sdk-metrics-base/src/export/MetricData.ts	`100.00% <ø> (ø)`
...-metrics-base/src/state/TemporalMetricProcessor.ts	`97.91% <ø> (ø)`
...ry-exporter-prometheus/src/PrometheusSerializer.ts	`95.12% <100.00%> (ø)`
...metrics-base/src/state/MeterProviderSharedState.ts	`100.00% <100.00%> (ø)`
...try-sdk-metrics-base/src/state/MeterSharedState.ts	`97.50% <100.00%> (ø)`
...etry-sdk-metrics-base/src/state/MetricCollector.ts	`100.00% <100.00%> (ø)`
...ckages/opentelemetry-sdk-metrics-base/src/utils.ts	`100.00% <100.00%> (ø)`
...elemetry-sdk-metrics-base/src/view/ViewRegistry.ts	`100.00% <100.00%> (ø)`
.../packages/otlp-transformer/src/metrics/internal.ts	`98.07% <100.00%> (ø)`

legendecas · 2022-05-10T15:03:03Z

 * feat: warn when hooked module is already loaded [#2926](https://github.com/open-telemetry/opentelemetry-js/pull/2926) @nozik
 * feat: implement OSDetector [#2927](https://github.com/open-telemetry/opentelemetry-js/pull/2927) @rauno56
 * feat: implement HostDetector [#2921](https://github.com/open-telemetry/opentelemetry-js/pull/2921) @rauno56
+* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc


There are two CHANGELOG files: one in the root directory and one in the exprimental/. I find that people tend to overlook the one in the experimental/ directory. Maybe we should merge those two.

I also picked the wrong one more than once. 😅

However, I added this entry in both CHANGELOG.md files intentionally as it adds InstrumentationScope to @opentelemetry/core (enhancement for stable), but also changes field names in the Metrics SDK (breaking for experimental).

in this case you should modify both changelog files like you did.

There are two CHANGELOG files: one in the root directory and one in the exprimental/. I find that people tend to overlook the one in the experimental/ directory. Maybe we should merge those two.

I thought about this, but I think it makes for a confusing changelog file.

Suggested change

* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc

* `@opentelemetry/core`: add InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc

Changed the changelog entry for stable in b5296d9 🙂

dyladan

Big PR but most renames like this are. LGTM and I don't think any were missed.

dyladan · 2022-05-10T20:26:53Z

 * feat: warn when hooked module is already loaded [#2926](https://github.com/open-telemetry/opentelemetry-js/pull/2926) @nozik
 * feat: implement OSDetector [#2927](https://github.com/open-telemetry/opentelemetry-js/pull/2927) @rauno56
 * feat: implement HostDetector [#2921](https://github.com/open-telemetry/opentelemetry-js/pull/2921) @rauno56
+* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc


in this case you should modify both changelog files like you did.

There are two CHANGELOG files: one in the root directory and one in the exprimental/. I find that people tend to overlook the one in the experimental/ directory. Maybe we should merge those two.

I thought about this, but I think it makes for a confusing changelog file.

dyladan · 2022-05-10T20:27:25Z

 * feat: warn when hooked module is already loaded [#2926](https://github.com/open-telemetry/opentelemetry-js/pull/2926) @nozik
 * feat: implement OSDetector [#2927](https://github.com/open-telemetry/opentelemetry-js/pull/2927) @rauno56
 * feat: implement HostDetector [#2921](https://github.com/open-telemetry/opentelemetry-js/pull/2921) @rauno56
+* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc


Suggested change

* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc

* `@opentelemetry/core`: add InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc

legendecas

Overall LGTM. Just a minor naming question.

legendecas · 2022-05-11T02:32:05Z


-export interface InstrumentationLibraryMetrics {
-  instrumentationLibrary: InstrumentationLibrary;
+export interface ScopeMetrics {


Is InstrumentationScopeMetrics a name to be considered? Although it is somewhat verbose, we are not introducing any new term in the case.

Oh, I found that this is the name in the OTLP proto.

Oh, I found that this is the name in the OTLP proto.

I considered InstrumentationScopeMetrics at first, but I think having this to as close as possible to the OTLP proto would make sense. 🙂

I checked the other language SDKs but there does not seem to be a consensus on what to call it. Java uses this MetricData to send to the Exporter, .NET does not map the concept directly, and Python is currently in the process of changing it to ScopeMetrics. If that Python PR merges, we will have consistent naming across Python, and JS and proto. 🙂

I think its fine if we're matching the proto

…done.

pichlermarc added 2 commits May 10, 2022 16:23

feat(opentelemetry-core): introduce InstrumentationScope.

b83bcbe

refactor(metrics): replace usage of InstrumentationScope with Instrum…

ae8dfbb

…entationLibrary.

pichlermarc changed the title ~~refactor(metrics-sdk): rename InstrumenationLibrary -> InstrumenationScope~~ refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope May 10, 2022

fix(changelog): update changelog.

7d777eb

pichlermarc marked this pull request as ready for review May 10, 2022 15:00

pichlermarc requested a review from a team May 10, 2022 15:00

legendecas reviewed May 10, 2022

View reviewed changes

dyladan approved these changes May 10, 2022

View reviewed changes

legendecas reviewed May 11, 2022

View reviewed changes

legendecas approved these changes May 11, 2022

View reviewed changes

pichlermarc added 2 commits May 11, 2022 10:47

fix(changelog): update (stable) changelog to better reflect what was …

b5296d9

…done.

Merge branch 'main' into rename-instrumentation-library

f37bc81

legendecas merged commit 97bc632 into open-telemetry:main May 11, 2022

dyladan deleted the rename-instrumentation-library branch May 11, 2022 14:54

dustinlessard-wf mentioned this pull request Nov 14, 2022

O11Y-2256 : Resource can be passed into MeterProvider Workiva/opentelemetry-dart#90

Merged

2 tasks

	* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc
	* `@opentelemetry/core`: add InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc

Uh oh!

Conversation

pichlermarc commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

codecov Bot commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

legendecas May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dyladan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legendecas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pichlermarc May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pichlermarc commented May 10, 2022 •

edited

Loading

codecov Bot commented May 10, 2022 •

edited

Loading

legendecas May 10, 2022 •

edited

Loading

pichlermarc May 11, 2022 •

edited

Loading