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

refactor(exporters): introduce packages for shared exporter classes #2893

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Apr 8, 2022

Which problem is this PR solving?

Currently the metrics exporters depend on the trace exporters, which causes unwanted dependencies to Trace packages via the Metrics exporters. This PR is part of the work to get rid of these dependencies by introducing new packages for Trace-independent export code that was previously exported from the exporter-trace-otlp-grpc, exporter-trace-otlp-http and exporter-trace-otlp-proto packages.

These shared classes have been moved to otlp-exporter-base, otlp-grpc-exporter-base and otlp-proto-exporter-base. Therefore,

  • opentelemetry-exporter-metrics-otlp-http
  • opentelemetry-exporter-metrics-otlp-proto
  • opentelemetry-exporter-metrics-otlp-grpc
  • exporter-trace-otlp-grpc
  • exporter-trace-otlp-http
  • exporter-trace-otlp-proto

have been updated to make use of these new packages instead.

While the exporters still depend on exporter-trace-otlp-http for transformation of the internal types to the proto-representation, this can be handled in a follow-up PR.

Related Issue: #2886

Short description of the changes

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
    • since previously exported classes have been moved to other packages. Other than that, this is a pure refactor.

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

  • Unit tests moved over from the original packages.

Checklist:

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

Sorry, something went wrong.

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #2893 (b45d71a) into main (0f3e4de) will decrease coverage by 0.92%.
The diff coverage is 80.32%.

@@            Coverage Diff             @@
##             main    #2893      +/-   ##
==========================================
- Coverage   93.73%   92.81%   -0.93%     
==========================================
  Files         187      184       -3     
  Lines        6145     6066      -79     
  Branches     1308     1296      -12     
==========================================
- Hits         5760     5630     -130     
- Misses        385      436      +51     
Impacted Files Coverage Δ
...packages/exporter-trace-otlp-http/src/transform.ts 97.39% <ø> (+8.69%) ⬆️
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 57.14% <ø> (ø)
...metrics-otlp-http/src/OTLPMetricExporterOptions.ts 100.00% <ø> (ø)
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts 95.00% <ø> (ø)
...ages/otlp-exporter-base/src/platform/node/types.ts 100.00% <ø> (ø)
...perimental/packages/otlp-exporter-base/src/util.ts 83.33% <ø> (ø)
...ntal/packages/otlp-grpc-exporter-base/src/types.ts 100.00% <ø> (ø)
...erimental/packages/otlp-exporter-base/src/types.ts 16.66% <16.66%> (ø)
...kages/otlp-exporter-base/src/platform/node/util.ts 27.58% <33.33%> (ø)
...ental/packages/otlp-grpc-exporter-base/src/util.ts 68.25% <72.22%> (ø)
... and 21 more

@morigs
Copy link
Contributor

morigs commented Apr 14, 2022

@pichlermarc hi, could you please provide an update on this? I wonder what's the ETA. Thx

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dyladan Daniel Dyla
@pichlermarc
Copy link
Member Author

@pichlermarc hi, could you please provide an update on this? I wonder what's the ETA. Thx

I'm not in the office this week, so I handed this PR over to @dyladan while I'm away. It is pretty much done though, but when I left some checks were failing.

@dyladan
Copy link
Member

dyladan commented Apr 15, 2022

There is still some minor cleanup but i'll mark it as ready for review so we can get some 👀 on it

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@dyladan dyladan marked this pull request as ready for review April 15, 2022 13:07
@dyladan dyladan requested a review from a team April 15, 2022 13:07

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dyladan Daniel Dyla
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm, since this one is huge i think it would be better to have more than 2 approvers to approve it for it to be merged

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

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

Thanks 🙏 A few minor comments

dyladan added 4 commits April 18, 2022 11:12

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dyladan Daniel Dyla

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dyladan Daniel Dyla

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dyladan Daniel Dyla

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dyladan Daniel Dyla
…er-base
@kumarrishav
Copy link

Hi team,
Are we good here to move forward?

pichlermarc and others added 2 commits April 19, 2022 14:26

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Big PR and tough to see if there are small things to clean up but tests are passing and it LGTM generally.

pichlermarc and others added 2 commits April 20, 2022 11:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@blumamir
Copy link
Member

Currently the metrics exporters depend on the trace exporters, which causes unwanted dependencies to Trace packages via the Metrics exporters

What is the plan for the future for these exporters? do we plan to keep them separated or merge them into a single exporter per-protocol that can export all stable signals (trace, metrics, and logs in the future)?

I don't see any good reason to separate them once metrics is stable, in which case we will probably need to merge these new packages back soon, right?
If we do want them separated in the future as well, should we also split the otlp-transformer package into per-signal packages?

@dyladan
Copy link
Member

dyladan commented Apr 20, 2022

As far as I know the exporters were split by signal in order to keep bundle size down. This was originally done by @obecny. Personally I think the developer experience is better to have a single exporter for all signals, but I'm not sure how much this affects the bundle size. Maybe @MSNev has some idea?

@pichlermarc
Copy link
Member Author

Currently the metrics exporters depend on the trace exporters, which causes unwanted dependencies to Trace packages via the Metrics exporters

What is the plan for the future for these exporters? do we plan to keep them separated or merge them into a single exporter per-protocol that can export all stable signals (trace, metrics, and logs in the future)?

I think this depends on if we want to support signals being used independently (see @dyladan's answer on bundle size). Regardless of that, I think keeping these new packages until the Logs SDK is stable is necessary if we want to keep experimental code in experimental/.

I don't see any good reason to separate them once metrics is stable, in which case we will probably need to merge these new packages back soon, right? If we do want them separated in the future as well, should we also split the otlp-transformer package into per-signal packages?

I think we would have to split otlp-transformer into three (utils, traces, metrics or four, with logs) as well, yes. otlp-transformer depends on sdk-metrics-base and sdk-trace-base right now.

@obecny
Copy link
Member

obecny commented Apr 20, 2022

When it was used for example in aws lambda (if I remember correctly) you don't want to have many MB as there is already a limit for your function. The same applies for browser you don't want to load many unneeded MB or kB of code. So hence the decision to split them so the user can include only what is necessary

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

👍 this is a great job!

@legendecas
Copy link
Member

Even if we merged trace/metrics OTLP exporters into one stable package, I think in the case of reducing bundle size in certain environments, people could still use tools to tree-shaking unused code components.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@dyladan dyladan merged commit e5031bd into open-telemetry:main Apr 21, 2022
@dyladan dyladan deleted the introduce-otlp-exporter-base branch April 21, 2022 12:49
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.

None yet

10 participants