refactor(sdk-metrics) Swap workaround types for @otel/api types #5254

chancancode · 2024-12-11T19:57:01Z

Which problem is this PR solving?

The internal Gauge and the unnamed MetricAdvice types were introduced in the 1.0 series when the metrics package depended on an older version of the api package that didn't come with those types, so the interfaces were essentially vendored into the package for internal use.

Now with the 2.0 release on the horrizon, we can bump the required version of @otel/api and is now safe to unvendor these types in favor of the official ones.

This is not expected to have any impact on consumers as the types are intended to be compatible for the positions they were used in.

However, the minimum requirement for the @opentelemetry/api peer dependency has been raised to 1.9.0.

Fixes #5223
Fixes #5224

Short description of the changes

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

TypeScript compiles

Checklist:

Followed the style guidelines of this project
~~Unit tests have been added~~ N/A – type-only changes
Documentation has been updated

linux-foundation-easycla · 2024-12-11T19:57:05Z

The committers listed above are authorized under a signed CLA.

✅ login: chancancode / name: Godfrey Chan (fd2f57c)

packages/sdk-metrics/package.json

chancancode · 2024-12-11T20:01:21Z

packages/sdk-metrics/src/InstrumentDescriptor.ts

   *
-   * This is intentionally not using the API's type as it's only available from @opentelemetry/api 1.7.0 and up.
-   * In SDK 2.0 we'll be able to bump the minimum API version and remove this workaround.
+   * @experimental


Kept the @experimental tag since the upstream type is itself @experimental. If it's not worth it I can also just remove the doc comment altogether.

FWIW, the entire InstrumentDescriptor interface is marked as @deprecated in sdk-metrics/index.ts in favor of MetricDescriptor, which does not have this field. But since @pichlermarc opened #5224 pretty recently, I assume the intention is to still keep this and carry it forward into 2.0.

Keeping it experimental is good - we'll bump it to stable in a separate issue. 👍

The InstrumentDescriptor is marked as deprecated as we will stop exporting it. Most properties will move to a MetricDescriptor and InstrumentDescriptor will extend it for internal use. Issue to follow.

codecov · 2024-12-12T10:41:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.57%. Comparing base (47212bb) to head (fd2f57c).
Report is 2 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #5254      +/-   ##
==========================================
+ Coverage   94.56%   94.57%   +0.01%     
==========================================
  Files         316      316              
  Lines        8037     8037              
  Branches     1628     1628              
==========================================
+ Hits         7600     7601       +1     
+ Misses        437      436       -1

Files with missing lines	Coverage Δ
packages/sdk-metrics/src/InstrumentDescriptor.ts	`100.00% <100.00%> (ø)`
packages/sdk-metrics/src/Instruments.ts	`96.00% <ø> (ø)`
packages/sdk-metrics/src/Meter.ts	`100.00% <ø> (ø)`

... and 1 file with indirect coverage changes

pichlermarc

Hi @chancancode, thank you for your contribution 🙂 This looks great already 🙌

Only a changelog entry in CHANGELOG_NEXT.md is missing then we can get this merged. 🙂

packages/sdk-metrics/package.json

pichlermarc · 2024-12-12T10:33:58Z

packages/sdk-metrics/src/InstrumentDescriptor.ts

   *
-   * This is intentionally not using the API's type as it's only available from @opentelemetry/api 1.7.0 and up.
-   * In SDK 2.0 we'll be able to bump the minimum API version and remove this workaround.
+   * @experimental


Keeping it experimental is good - we'll bump it to stable in a separate issue. 👍

The InstrumentDescriptor is marked as deprecated as we will stop exporting it. Most properties will move to a MetricDescriptor and InstrumentDescriptor will extend it for internal use. Issue to follow.

chancancode · 2024-12-12T18:38:05Z

@pichlermarc Should be good to go now. Categorized it as a refactor since these types are internal and the replacements are expected to be compatible. Since I have the context open now, wouldn't mind working on that next refactor, my understanding is (on the next branch):

remove the deprecated alias export in sdk-metrics/index.ts
refactor the internal un-exported InstrumentDescriptor to extend from MetricDescriptor
add CHANGELOG_NEXT.md entry

If that sounds good, let me know, save you from creating an issue for it.

pichlermarc

thank you for working on this 🙂

pichlermarc · 2024-12-13T11:05:56Z

Since I have the context open now, wouldn't mind working on that next refactor, my understanding is (on the next branch):
* remove the deprecated alias export in `sdk-metrics/index.ts`

* refactor the internal un-exported `InstrumentDescriptor` to extend from `MetricDescriptor`

* add `CHANGELOG_NEXT.md` entry
If that sounds good, let me know, save you from creating an issue for it.

That sounds great 🙌

pichlermarc · 2024-12-13T11:02:40Z

CHANGELOG_NEXT.md

@@ -23,6 +23,7 @@

 ### :house: (Internal)

+* refactor(sdk-metrics): remove `Gauge` and `MetricAdvice` workaround types in favor of the upstream `@opentelemetry/api` types [#5254](https://github.com/open-telemetry/opentelemetry-js/pull/5254) @chancancode


ah there still needs to be an entry to breaking as we drop support for old API versions 🙂

Oops, that's right, fixed!

The internal `Gauge` and the unnamed `MetricAdvice` types were introduced in the 1.0 series when the metrics package depended on an older version of the api package that didn't come with those types, so the interfaces were essentially vendored into the package for internal use. Now with the 2.0 release on the horrizon, we can bump the required version of @otel/api and is now safe to unvendor these types in favor of the official ones. This is not expected to have any impact on consumers as the types are intended to be compatible for the positions they were used in. However, the minimum requirement for the `@opentelemetry/api` peer dependency has been raised to 1.9.0. Fixes open-telemetry#5223 Fixes open-telemetry#5224

chancancode requested a review from a team as a code owner December 11, 2024 19:57

chancancode commented Dec 11, 2024

View reviewed changes

packages/sdk-metrics/package.json Show resolved Hide resolved

chancancode commented Dec 11, 2024

View reviewed changes

pichlermarc reviewed Dec 12, 2024

View reviewed changes

chancancode force-pushed the rm-workaround-types branch 3 times, most recently from 3f2a9fa to 230d3eb Compare December 12, 2024 18:26

chancancode changed the title ~~[sdk-metrics] Switch workaround types to official @otel/api types~~ refactor(sdk-metrics) Swap workaround types for @otel/api types Dec 12, 2024

pichlermarc approved these changes Dec 13, 2024

View reviewed changes

pichlermarc reviewed Dec 13, 2024

View reviewed changes

chancancode force-pushed the rm-workaround-types branch from 230d3eb to 16b6389 Compare December 13, 2024 17:24

chancancode force-pushed the rm-workaround-types branch from 16b6389 to fd2f57c Compare December 13, 2024 17:25

pichlermarc approved these changes Dec 13, 2024

View reviewed changes

pichlermarc merged commit 6d7a73a into open-telemetry:next Dec 13, 2024
18 checks passed

chancancode deleted the rm-workaround-types branch December 13, 2024 18:19

This was referenced Dec 13, 2024

feat(sdk-metrics)!: Drop deprecated InstrumentDescriptor export #5266

Merged

[sdk-metrics] remove Gauge type workaround #5223

Closed

[sdk-metrics] remove MetricAdvice type workaround #5224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sdk-metrics) Swap workaround types for @otel/api types #5254

refactor(sdk-metrics) Swap workaround types for @otel/api types #5254

chancancode commented Dec 11, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 11, 2024 •

edited

Loading

chancancode Dec 11, 2024 •

edited

Loading

pichlermarc Dec 12, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

pichlermarc left a comment

pichlermarc Dec 12, 2024

chancancode commented Dec 12, 2024

pichlermarc left a comment

pichlermarc commented Dec 13, 2024

pichlermarc Dec 13, 2024

chancancode Dec 13, 2024

		@@ -23,6 +23,7 @@

		### :house: (Internal)

		* refactor(sdk-metrics): remove `Gauge` and `MetricAdvice` workaround types in favor of the upstream `@opentelemetry/api` types [#5254](https://github.com/open-telemetry/opentelemetry-js/pull/5254) @chancancode

refactor(sdk-metrics) Swap workaround types for @otel/api types #5254

refactor(sdk-metrics) Swap workaround types for @otel/api types #5254

Conversation

chancancode commented Dec 11, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Dec 11, 2024 • edited Loading

chancancode Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

pichlermarc Dec 12, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc Dec 12, 2024

Choose a reason for hiding this comment

chancancode commented Dec 12, 2024

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc commented Dec 13, 2024

pichlermarc Dec 13, 2024

Choose a reason for hiding this comment

chancancode Dec 13, 2024

Choose a reason for hiding this comment

chancancode commented Dec 11, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 11, 2024 •

edited

Loading

chancancode Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading