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

grpc for node and support for new proto format for node and browser #901

Merged
merged 20 commits into from Apr 9, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Mar 25, 2020

Which problem is this PR solving?

Short description of the changes

  • It adds grpc for node
  • It uses new proto format for browser and node

Bear in mind that currently there is no yet opentelemetry collector that supports grpc and new format, this needs to wait until this is done
it is there now, tested and fixed

@obecny obecny self-assigned this Mar 25, 2020
@obecny obecny added the enhancement New feature or request label Mar 25, 2020
@codecov-io
Copy link

codecov-io commented Mar 25, 2020

Codecov Report

Merging #901 into master will decrease coverage by 0.25%.
The diff coverage is 78.49%.

@@            Coverage Diff             @@
##           master     #901      +/-   ##
==========================================
- Coverage   94.94%   94.69%   -0.26%     
==========================================
  Files         247      247              
  Lines       11142    11228      +86     
  Branches     1065     1077      +12     
==========================================
+ Hits        10579    10632      +53     
- Misses        563      596      +33     
Impacted Files Coverage Δ
...es/opentelemetry-exporter-collector/test/helper.ts 68.47% <67.04%> (-31.53%) ⬇️
.../opentelemetry-exporter-collector/src/transform.ts 86.07% <85.07%> (-3.67%) ⬇️
...emetry-exporter-collector/src/CollectorExporter.ts 90.47% <90.00%> (-0.83%) ⬇️
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <100.00%> (ø)
...es/opentelemetry-exporter-collector/src/version.ts 0.00% <0.00%> (-100.00%) ⬇️

@mayurkale22
Copy link
Member

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

@obecny
Copy link
Member Author

obecny commented Mar 30, 2020

Wait with more reviews as I can't make it work with latest otel anyway - will let you know where is the problem when I finish

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.

Requesting changes so this doesn't get accidentally merged while @obecny is still working on it.

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.

Added first pass comments.

.gitmodules Show resolved Hide resolved
packages/opentelemetry-core/src/platform/node/index.ts Outdated Show resolved Hide resolved
packages/opentelemetry-exporter-collector/package.json Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member

@obecny why do you think this one has breaking changes?

@obecny
Copy link
Member Author

obecny commented Apr 7, 2020

@obecny why do you think this one has breaking changes?
because you won't be able to use the new version with old collector - format is different and grpc is being used instead of http

@dyladan
Copy link
Member

dyladan commented Apr 7, 2020

I agree with @obecny on breaking from the standpoint that if you have a previously running collector that we are exporting to and you update with this PR version, it may stop working.

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.

Overall LGTM. Honestly, PR is very big, easy to miss things. I would like to give another pass before approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants