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

feat(api): merge api-metrics into api #3374

Merged
merged 9 commits into from
Nov 3, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Nov 2, 2022

Which problem is this PR solving?

Merge stable API packages @opentelemetry/api-metrics into @opentelemetry/api.

Fixes #3368

Notably, those packages that depend on the metrics API now declare peer dependency on @opentelemetry/api ">=1.2.0 <1.3.0" and "^1.,2.0". The version range should be updated to ">=1.3.0 <1.4.0" and "^1.3.0" respectively when metrics API is released in @opentelemetry/api v1.3.0.

Type of change

  • 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?

  • Tree-shaking tests in @opentelemetry/api.

Checklist:

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

@legendecas legendecas requested a review from a team November 2, 2022 03:20
@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #3374 (a1aeb33) into main (bdc603c) will decrease coverage by 0.02%.
The diff coverage is 98.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3374      +/-   ##
==========================================
- Coverage   93.26%   93.24%   -0.03%     
==========================================
  Files         246      247       +1     
  Lines        7413     7341      -72     
  Branches     1533     1509      -24     
==========================================
- Hits         6914     6845      -69     
+ Misses        499      496       -3     
Impacted Files Coverage Δ
api/src/internal/global-utils.ts 90.62% <ø> (ø)
api/src/metrics/Metric.ts 100.00% <ø> (ø)
api/src/metrics/NoopMeter.ts 97.36% <ø> (ø)
api/src/metrics/NoopMeterProvider.ts 100.00% <ø> (ø)
...emetry-instrumentation-grpc/src/instrumentation.ts 75.86% <0.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 94.77% <ø> (-0.02%) ⬇️
...es/opentelemetry-instrumentation-http/src/utils.ts 99.17% <ø> (ø)
...entelemetry-instrumentation/src/autoLoaderUtils.ts 92.00% <ø> (ø)
...entelemetry-instrumentation/src/instrumentation.ts 69.23% <ø> (-1.14%) ⬇️
.../opentelemetry-sdk-metrics/src/aggregator/types.ts 100.00% <ø> (ø)
... and 30 more

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

I think merging these packages is the right way to go 🙂
Looks good 🙂

api/src/context-api.ts Show resolved Hide resolved
experimental/packages/api-logs/README.md Show resolved Hide resolved
api/src/index.ts Outdated
export { HrTime, TimeInput } from './common/Time';
export { Attributes, AttributeValue } from './common/Attributes';

export * from './context-api';
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have this as explicit (named) exports rather than implicit exports (*) to remove the unexpected inclusion of exported values from the listed files.

Copy link
Member Author

@legendecas legendecas Nov 2, 2022

Choose a reason for hiding this comment

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

I have updated the exports in *-api.ts file to be explicit so I export everything from *-api.ts from index.ts. Like trace-api.ts, which used to be export * from each internal module.

In this way, we can have a relatively short index.ts and check *-api.ts for the exports of each API.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @MSNev here. The index is the one source of truth to see what is exported from the API package. It is not convenient to have to go to another file to see what is exported. Also, someone may add an export to that file not realizing that index.ts has the * export.

Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

Left a few minor suggestions..

I REALLY like the removal of the namespace references api.xxx etc, this is one of the things I was planning for the web sandbox because of the tree-shaking improvements that this has.

@legendecas
Copy link
Member Author

@MSNev @dyladan updated with explicit exports moved to index.ts. PTAL :)

Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

👍

@dyladan dyladan merged commit 1536a7c into open-telemetry:main Nov 3, 2022
@legendecas legendecas deleted the merge-api-metrics branch November 4, 2022 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merging stable API packages
5 participants