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

chore: cleanup API dependencies #2904

Merged
merged 4 commits into from
Apr 20, 2022
Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Apr 19, 2022

Which problem is this PR solving?

Address unresolved comments from #2892.

Short description of the changes

Move api-metrics into dependency instead of mixing dep/peer-dep.
Use fixed version for components part of the lerna project.
Move api-metrics to dev-dependency in exporter prometheus.

Type of change

How Has This Been Tested?

CI, local

Checklist:

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

Move api-metrics into dependency instead of mixing dep/peer-dep.
Use fixed version for components part of the lerna project.
Move api-metrics to dev-dependency in exporter prometheus.
@Flarna Flarna requested a review from a team April 19, 2022 06:59
@Flarna
Copy link
Member Author

Flarna commented Apr 19, 2022

An alternative would be to move api-metrics into peer-dep like api at all places.

Please note also that api-metrics has api as a dependency. Therefore having api-metrics as a dependency one has automatically api as dependency (not as peer-dependency like if metrics is not used).

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #2904 (0846ce1) into main (75cbf5d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2904   +/-   ##
=======================================
  Coverage   93.73%   93.73%           
=======================================
  Files         187      187           
  Lines        6145     6145           
  Branches     1308     1308           
=======================================
  Hits         5760     5760           
  Misses        385      385           
Impacted Files Coverage Δ
...ry-exporter-prometheus/src/PrometheusSerializer.ts 95.12% <ø> (ø)

@@ -45,6 +45,7 @@
},
"devDependencies": {
"@opentelemetry/api": "^1.0.0",
"@opentelemetry/api-metrics": "0.27.0",
Copy link
Member

Choose a reason for hiding this comment

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

Does this also need to be added to peer dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

Used in src/PrometheusSerializer.ts line 27

Copy link
Member

Choose a reason for hiding this comment

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

Or real dependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

It imports only a type import type { MetricAttributes } from '@opentelemetry/api-metrics'; and the place where it is used is not exported. Therefore it's only needed at compile time.

Copy link
Member

Choose a reason for hiding this comment

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

I think when users depend on the exporter the typescript libcheck will still look for api-metrics even just for types won't it? The API will definitely be there because it would be useless to have the exporter without it, but I think it is not strictly a dev dependency when it is type only. I think this is the same thing that happened in contrib when the packages being instrumented have their types imported.

Copy link
Member Author

Choose a reason for hiding this comment

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

The lib check will only complain if it is part of an exported API or inside a file which is part of the exported API.

But the serialize function is internal and therefore not even included in PrometheusSerializer.d.ts. Maybe you remember open-telemetry/opentelemetry-js-contrib#932 where I solved a similar problem by moving the internal types into an internal file.

But as metrics-api will be present anyway because of other deps it's maybe better to move it back. This protects us also from accidentally run into a libcheck problem in future just by using a type somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good thanks for the explanation

@vmarchaud vmarchaud merged commit 0f3e4de into open-telemetry:main Apr 20, 2022
@Flarna Flarna deleted the cleanup-deps branch April 20, 2022 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants