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: remove grpc trace exporter dependency on grpc-exporter-base #3100

Closed
wants to merge 23 commits into from

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jul 18, 2022

Todo in follow-up:

  • Apply the same changes to the metrics exporter
  • Remove base packages
  • Pre-compile proto files to JS in order to more easily support webpack
  • Possibly browser support?

@dyladan dyladan requested a review from a team July 18, 2022 12:30
@dyladan dyladan force-pushed the trace-metric-grpc branch from d035aeb to c482bcb Compare July 18, 2022 12:31
@dyladan
Copy link
Member Author

dyladan commented Jul 18, 2022

@open-telemetry/javascript-maintainers there is one failing test but I actually believe the test is bad. It is asserting that if you pass metadata to the constructor, it is overridden by the environment variable. I think code should always override env config. All other tests are passing (locally at least, still waiting for CI).

Decided to keep the test for now and apply the change to all exporters at the same time later.

@dyladan dyladan marked this pull request as draft July 18, 2022 12:56
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #3100 (fdc2800) into main (d5cf120) will decrease coverage by 0.93%.
The diff coverage is n/a.

❗ Current head fdc2800 differs from pull request most recent head 3f9dad2. Consider uploading reports for the commit 3f9dad2 to get more accurate results

@@            Coverage Diff             @@
##             main    #3100      +/-   ##
==========================================
- Coverage   93.08%   92.14%   -0.94%     
==========================================
  Files         188       82     -106     
  Lines        6244     2407    -3837     
  Branches     1313      522     -791     
==========================================
- Hits         5812     2218    -3594     
+ Misses        432      189     -243     
Impacted Files Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 31.81% <0.00%> (-68.19%) ⬇️
...perimental/packages/otlp-exporter-base/src/util.ts 79.41% <0.00%> (-17.65%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 93.33% <0.00%> (-6.67%) ⬇️
...exporter-trace-otlp-proto/src/OTLPTraceExporter.ts
...elemetry-sdk-metrics-base/src/export/MetricData.ts
... and 103 more

@dyladan dyladan marked this pull request as ready for review July 18, 2022 18:25
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.

Just a few comments. Also a bit worried about the coverage of the security config code, as touching that code caused the exporters to break the last time around.

@dyladan
Copy link
Member Author

dyladan commented Jul 19, 2022

Just a few comments. Also a bit worried about the coverage of the security config code, as touching that code caused the exporters to break the last time around.

Brought that code over from the base package and forgot to bring the tests. good catch

@dyladan dyladan force-pushed the trace-metric-grpc branch from 47e30e3 to b60ca26 Compare July 19, 2022 19:38
@dyladan
Copy link
Member Author

dyladan commented Jul 20, 2022

@pichlermarc added some tests for the security module

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.

Looks good. Thanks for adding the tests 🙂

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 but i'm not sure to understand why using the base package were breaking the exports ?

@dyladan
Copy link
Member Author

dyladan commented Jul 25, 2022

@vmarchaud actually it looks like you're right. A simple test shows that the serviceClient isn't being overwritten like I thought. My initial thought was that because the onInit happened async it was being called from the super class after the child classes had already been loaded and that caused it to be overwritten but I can't reproduce it.

@dyladan
Copy link
Member Author

dyladan commented Jul 25, 2022

Going to close this since the issue was fixed in a different PR

@dyladan dyladan closed this Jul 25, 2022
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.

3 participants