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: adding proto over http for collector exporter #1302

Merged
merged 13 commits into from
Jul 22, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Jul 10, 2020

Which problem is this PR solving?

Short description of the changes

  • It adds a new transport layer for proto over http for Collector Exporter

@obecny obecny added the size/XL label Jul 10, 2020
@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #1302 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1302   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files         193      193           
  Lines        4804     4807    +3     
  Branches      982      983    +1     
=======================================
+ Hits         4453     4456    +3     
  Misses        351      351           
Impacted Files Coverage Δ
opentelemetry-exporter-collector/src/types.ts 100.00% <0.00%> (ø)

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.

overall lgtm, however i'm not sure we should commit the generated files. Could we just generate them when running the compile step ?

@obecny
Copy link
Member Author

obecny commented Jul 13, 2020

overall lgtm, however i'm not sure we should commit the generated files. Could we just generate them when running the compile step ?

I don't have a strong opinion on that but to generate proto you need to install protocol buffers and they are not not available on all platforms, (only windows, linux, osx). So having this in mind you might not necessary want to install protocol buffers to be able to generate js

@naseemkullah
Copy link
Member

overall lgtm, however i'm not sure we should commit the generated files. Could we just generate them when running the compile step ?
I don't have a strong opinion on that but to generate proto you need to install protocol buffers and they are not not available on all platforms, (only windows, linux, osx). So having this in mind you might not necessary want to install protocol buffers to be able to generate js

FWIW
What I've seen in another OSS project (linkerd), is a script to run to generate the files at will: https://github.com/linkerd/linkerd2/blob/main/BUILD.md#updating-protobuf-dependencies
https://github.com/linkerd/linkerd2/blob/main/bin/protoc-go.sh

Not sure if this would be applicable here but thought it worth mentioning.

@dyladan
Copy link
Member

dyladan commented Jul 13, 2020

I don't have a strong opinion on that but to generate proto you need to install protocol buffers and they are not not available on all platforms, (only windows, linux, osx). So having this in mind you might not necessary want to install protocol buffers to be able to generate js

This is only for local development right? The target who installs the published package does not need to have it installed? If that is correct, I think osx/win/linux is an acceptable limitation.

@obecny
Copy link
Member Author

obecny commented Jul 13, 2020

I don't have a strong opinion on that but to generate proto you need to install protocol buffers and they are not not available on all platforms, (only windows, linux, osx). So having this in mind you might not necessary want to install protocol buffers to be able to generate js

This is only for local development right? The target who installs the published package does not need to have it installed? If that is correct, I think osx/win/linux is an acceptable limitation.

You are using generated js files to serialize data. So if files are generated it doesn't matter which platform you have in production as it will work fine. But to be able to generate them (for example after updating proto submodule to newer version) you need to regenerate them again manually together with submodule update and then commit changes together. And of course to be able to generate them you need to have protoc locally

@dyladan
Copy link
Member

dyladan commented Jul 13, 2020

I think generating them in the compile step is acceptable. My issue is that sh scripts are not windows compatible.

@obecny
Copy link
Member Author

obecny commented Jul 13, 2020

I think generating them in the compile step is acceptable. My issue is that sh scripts are not windows compatible.

What alternative do we have to be able generate them in compile step then ?
That's why I would rather have them already in place for simplicity otherwise I'm open for suggestions if generating them in compile step is a must - although I would like to avoid it.

@dyladan
Copy link
Member

dyladan commented Jul 13, 2020

I think something like what naseem suggested might work. Check in the generated files and just have a defined process for how to update them

@obecny
Copy link
Member Author

obecny commented Jul 13, 2020

FWIW
What I've seen in another OSS project (linkerd), is a script to run to generate the files at will: https://github.com/linkerd/linkerd2/blob/main/BUILD.md#updating-protobuf-dependencies
https://github.com/linkerd/linkerd2/blob/main/bin/protoc-go.sh

Not sure if this would be applicable here but thought it worth mentioning.

I think something like what naseem suggested might work. Check in the generated files and just have a defined process for how to update them

But this is also a bash script that is using the same protoc :).

@dyladan
Copy link
Member

dyladan commented Jul 13, 2020

FWIW
What I've seen in another OSS project (linkerd), is a script to run to generate the files at will: linkerd/linkerd2@main/BUILD.md#updating-protobuf-dependencies
linkerd/linkerd2@main/bin/protoc-go.sh
Not sure if this would be applicable here but thought it worth mentioning.

I think something like what naseem suggested might work. Check in the generated files and just have a defined process for how to update them

But this is also a bash script that is using the same protoc :).

I mean the general idea of a script that doesn't run automatically but needs to be run manually. It should be fairly simple to use something like git-js and/or the child_process module to handle this in pure js.

@obecny
Copy link
Member Author

obecny commented Jul 13, 2020

I have experimented with profobuf js and it seems like this is working fine. Moreover it simplifies the whole setup a lot and in the end it becomes just a few lines of code . Are there any objections to use if instead of google protobuf ?

@dyladan
Copy link
Member

dyladan commented Jul 13, 2020

I don't have any objections but I am not an expert in protobuf

@obecny
Copy link
Member Author

obecny commented Jul 13, 2020

ok now the proto is based on protobufjs, much simpler

@vmarchaud
Copy link
Member

@obecny No link with this PR but do you think it would possible to replace grpc by grpc-js later ? I use the collector exporter and found out that grpc is quite heavy when building our docker images:

image

@obecny
Copy link
Member Author

obecny commented Jul 15, 2020

@obecny No link with this PR but do you think it would possible to replace grpc by grpc-js later ? I use the collector exporter and found out that grpc is quite heavy when building our docker images:

image

something to investigate what is really the best option here (new package, peer dependency etc.)

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.

Definitely better without the generated files

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM!

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 one minor comment, otherwise LGTM

@obecny obecny merged commit d6e96e0 into open-telemetry:master Jul 22, 2020
@obecny obecny deleted the collector_proto branch July 22, 2020 11:38
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
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
Development

Successfully merging this pull request may close these issues.

OTLP/HTTP support for Node
6 participants