feat: adding proto over http for collector exporter #1302

obecny · 2020-07-10T12:40:56Z

Which problem is this PR solving?

Fixes OTLP/HTTP support for Node #1242 - the second part proto over http

Short description of the changes

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

codecov · 2020-07-10T12:48:21Z

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%> (ø)`

vmarchaud

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

obecny · 2020-07-13T17:45:31Z

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 · 2020-07-13T17:48:07Z

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 · 2020-07-13T17:48:19Z

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 · 2020-07-13T17:54:45Z

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 · 2020-07-13T18:03:51Z

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

obecny · 2020-07-13T18:09:26Z

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 · 2020-07-13T18:30:58Z

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 · 2020-07-13T19:16:17Z

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 · 2020-07-13T19:28:50Z

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 · 2020-07-13T20:55:06Z

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 · 2020-07-13T21:09:05Z

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

obecny · 2020-07-13T23:01:49Z

ok now the proto is based on protobufjs, much simpler

vmarchaud · 2020-07-14T13:44:05Z

@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:

obecny · 2020-07-15T13:03:48Z

@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:

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

dyladan

Definitely better without the generated files

mwear

LGTM!

mayurkale22

Added one minor comment, otherwise LGTM

packages/opentelemetry-exporter-collector/src/platform/node/README.md

…pen-telemetry#1302)

feat: adding proto over http for collector exporter

99d8854

obecny requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, OlivierAlbertini and vmarchaud as code owners July 10, 2020 12:40

obecny self-assigned this Jul 10, 2020

obecny added the enhancement New feature or request label Jul 10, 2020

chore: lint

6638c32

obecny added the size/XL label Jul 10, 2020

obecny added 3 commits July 10, 2020 14:53

chore: lint

dbdd707

chore: fixing transport type in example

32a01c5

chore: adding generated to files in package.json

0b7dc81

vmarchaud approved these changes Jul 11, 2020

View reviewed changes

chore: adding more info about protoc

11c5a56

obecny added 3 commits July 14, 2020 00:21

chore: replacing google-protobuf with protobufjs

bfeaec7

chore: merge branch 'master' into collector_proto

7c450b6

chore: cleanup

18b0745

dyladan approved these changes Jul 17, 2020

View reviewed changes

obecny added 2 commits July 21, 2020 17:59

chore: merge with master

8fb0274

chore: fixes after merge

65b89e1

mwear approved these changes Jul 21, 2020

View reviewed changes

mayurkale22 approved these changes Jul 21, 2020

View reviewed changes

packages/opentelemetry-exporter-collector/src/platform/node/README.md Outdated Show resolved Hide resolved

obecny added 2 commits July 22, 2020 11:54

chore: clean up

09df9bc

Merge branch 'master' into collector_proto

9d8c6b7

obecny merged commit d6e96e0 into open-telemetry:master Jul 22, 2020

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

chore(aws-sdk): bump aws-sdk version to align with previous releases (o…

cd9984a

…pen-telemetry#1302)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding proto over http for collector exporter #1302

feat: adding proto over http for collector exporter #1302

obecny commented Jul 10, 2020 •

edited

Loading

codecov bot commented Jul 10, 2020 •

edited

Loading

vmarchaud left a comment

obecny commented Jul 13, 2020 •

edited

Loading

naseemkullah commented Jul 13, 2020

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020 •

edited

Loading

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020

dyladan commented Jul 13, 2020 •

edited

Loading

obecny commented Jul 13, 2020

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020

vmarchaud commented Jul 14, 2020

obecny commented Jul 15, 2020

dyladan left a comment

mwear left a comment

mayurkale22 left a comment

feat: adding proto over http for collector exporter #1302

feat: adding proto over http for collector exporter #1302

Conversation

obecny commented Jul 10, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 10, 2020 • edited Loading

Codecov Report

vmarchaud left a comment

Choose a reason for hiding this comment

obecny commented Jul 13, 2020 • edited Loading

naseemkullah commented Jul 13, 2020

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020 • edited Loading

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020

dyladan commented Jul 13, 2020 • edited Loading

obecny commented Jul 13, 2020

dyladan commented Jul 13, 2020

obecny commented Jul 13, 2020

vmarchaud commented Jul 14, 2020

obecny commented Jul 15, 2020

dyladan left a comment

Choose a reason for hiding this comment

mwear left a comment

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

obecny commented Jul 10, 2020 •

edited

Loading

codecov bot commented Jul 10, 2020 •

edited

Loading

obecny commented Jul 13, 2020 •

edited

Loading

obecny commented Jul 13, 2020 •

edited

Loading

dyladan commented Jul 13, 2020 •

edited

Loading