Skip to content

Conversation

@obecny
Copy link
Member

@obecny obecny commented Nov 20, 2019

Which problem is this PR solving?

  1. Fixes Add OpenTelemetry Collector exporter for web #499
  2. Fixes Add OpenTelemetry Collector exporter for Node.js #502

Short description of the changes

It adds new CollectorExporter for web and node. It will export spans either from web or node to opentelemetry-collector using the opencensus receiver. There is one docker compose file inside examples/basic-tracer-node/docker.

@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

Attention: Patch coverage is 69.26407% with 71 lines in your changes missing coverage. Please review.

Project coverage is 89.94%. Comparing base (c14d122) to head (b603098).

Files with missing lines Patch % Lines
...y-exporter-collector/test/common/transform.test.ts 0.00% 28 Missing ⚠️
...-exporter-collector/src/platform/node/sendSpans.ts 0.00% 19 Missing ⚠️
...kages/opentelemetry-core/scripts/version-update.js 0.00% 9 Missing ⚠️
...metry-exporter-collector/scripts/version-update.js 0.00% 9 Missing ⚠️
...emetry-exporter-collector/src/CollectorExporter.ts 93.61% 3 Missing ⚠️
.../opentelemetry-exporter-collector/src/transform.ts 96.96% 2 Missing ⚠️
...ntelemetry-core/src/platform/node/hex-to-base64.ts 88.88% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (69.26%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
- Coverage   90.16%   89.94%   -0.22%     
==========================================
  Files         146      168      +22     
  Lines        7390     8238     +848     
  Branches      631      739     +108     
==========================================
+ Hits         6663     7410     +747     
- Misses        727      828     +101     
Files with missing lines Coverage Δ
packages/opentelemetry-core/src/common/time.ts 95.38% <100.00%> (+0.46%) ⬆️
packages/opentelemetry-core/src/common/version.ts 100.00% <100.00%> (ø)
...ckages/opentelemetry-core/test/common/time.test.ts 100.00% <100.00%> (ø)
...telemetry-core/test/platform/hex-to-base64.test.ts 100.00% <100.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <100.00%> (ø)
...es/opentelemetry-exporter-collector/src/version.ts 100.00% <100.00%> (ø)
...y-exporter-collector/test/browser/index-webpack.ts 100.00% <100.00%> (ø)
...es/opentelemetry-exporter-collector/test/helper.ts 100.00% <100.00%> (ø)
...ntelemetry-core/src/platform/node/hex-to-base64.ts 88.88% <88.88%> (ø)
.../opentelemetry-exporter-collector/src/transform.ts 96.96% <96.96%> (ø)
... and 5 more

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@obecny
Copy link
Member Author

obecny commented Nov 20, 2019

cc @draffensperger

Copy link
Member

@mayurkale22 mayurkale22 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 great 💯, added few minor comments in the first pass. I will spend some time today to review the rest of the conversation stuff.

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Addded a few more comments.

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

Very cool! Have some comments but awesome to see this coming together.

annotation.attributes = attributes;
}

// const messageEvent: MessageEvent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we open a tracking issue for us to add message events? @mayurkale22 / @bg451 is that in the spec?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK we can use event.attributes to assign the MessageEventType (RECEIVED or SEND) and uncompressedSize and compressedSize bytes instead of adding dedicated api for message event. Maybe you should open an issue to define semantic conversions for all of these attributes.

annotation.attributes = attributes;
}

// const messageEvent: MessageEvent;
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK we can use event.attributes to assign the MessageEventType (RECEIVED or SEND) and uncompressedSize and compressedSize bytes instead of adding dedicated api for message event. Maybe you should open an issue to define semantic conversions for all of these attributes.

Copy link
Member

@bg451 bg451 left a comment

Choose a reason for hiding this comment

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

Awesome work @obecny!

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

LGTM, added a few minor comments. Great work! 💯

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 1, 2019
@mayurkale22
Copy link
Member

@open-telemetry/javascript-approvers Please review when you get a chance.

@mayurkale22 mayurkale22 requested review from bg451 and dyladan December 2, 2019 18:15
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.

🚀

@mayurkale22 mayurkale22 merged commit b58ad10 into open-telemetry:master Dec 4, 2019
@obecny obecny deleted the collector-exporter branch February 2, 2021 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add OpenTelemetry Collector exporter for Node.js Add OpenTelemetry Collector exporter for web

8 participants