Collector split

[obecny]

Which problem is this PR solving?

• Fixes Collector Exporter - split into 3 packages #1429

Short description of the changes

1. Splitting Collector into 3 pakages
   collector-exporter-node
   collector-exporter-node-proto
   collector-exporter-node-grpc

2. Updated examples for metrics and tracing including browser

[codecov]

Codecov Report

Merging #1446 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1446      +/-   ##
==========================================
+ Coverage   93.71%   93.74%   +0.02%     
==========================================
  Files         149      149              
  Lines        4503     4505       +2     
  Branches      930      929       -1     
==========================================
+ Hits         4220     4223       +3     
+ Misses        283      282       -1     

Impacted Files	Coverage Δ
...ages/opentelemetry-exporter-collector/src/types.ts	100.00% <ø> (ø)
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts	81.42% <100.00%> (+1.12%)	⬆️
...opentelemetry-core/src/platform/node/BasePlugin.ts	81.08% <100.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts	96.77% <100.00%> (+1.07%)	⬆️
...es/opentelemetry-metrics/src/BaseObserverMetric.ts	100.00% <100.00%> (ø)
...s/opentelemetry-metrics/src/BatchObserverMetric.ts	93.10% <100.00%> (ø)
...kages/opentelemetry-metrics/src/BoundInstrument.ts	100.00% <100.00%> (ø)
...ges/opentelemetry-metrics/src/SumObserverMetric.ts	100.00% <100.00%> (ø)
...s/opentelemetry-metrics/src/UpDownCounterMetric.ts	100.00% <100.00%> (ø)
...entelemetry-metrics/src/UpDownSumObserverMetric.ts	100.00% <100.00%> (ø)
... and 3 more

[vmarchaud]

I think we should name the proto package http instead to make it more clear which protocol it use. WDYT ?

[dyladan]

This is a large PR and a little difficult to review, but it seems generally right to me.

I think we should name the proto package http instead to make it more clear which protocol it use. WDYT ?

That might be confusing because we have json over http and proto over http.

[vmarchaud]

That might be confusing because we have json over http and proto over http.

Agree, then why not proto-http, grpc and json-http ?

[obecny]

That might be confusing because we have json over http and proto over http.

Agree, then why not proto-http, grpc and json-http ?

The base is collector-exporter-node it is a json over http for node and json over sendbeacon or xmlhttprequest for web.
Having a name "json-http" would be misleading. Having said that then proto-http could confuse people who will try to find json over xmlhttp etc. We also have grpc which is I think straightforward and to distinguish grpc from proto in the most easy way is to simply say proto without any extra context.

I have made proposition for naming in the corresponding issue 1429 and because no1 complained since then I used the proposed naming. Trying to change it now especially when you have submodules that were already added and then changing paths and name in many places is just painful. I would like to leave the name as they are.

[dyladan]

I think the name is fine as long as the readme is clear that the module exports protobuf over http

[dyladan]

Sorry this review took so long. Even just the .ts files is almost 50 files changed, even if it is mostly existing code that has been split.

I think the names are fine, but it would probably be nice to have documented somewhere how to choose between the 3 exporter options for people who are less familiar with the various transports and collector in general.

[mwear]

Thanks @obecny!

[mayurkale22]

Although overall looks good and we are splitting the existing work, I am still a little nervous to review this one. Perhaps, in the future we should split this kind of large work into smaller PRs (if possible).

[vmarchaud]

lgtm

[vmarchaud]

I tested with the collector v0.8.0 for both metrics and traces and the exporter works, if you want to update this

[vmarchaud]

Update: there are some errors with the metrics exporter and collector v0.8.0, i'm trying to pinpoint them

[obecny]

It might be as collector is changing all the time as well as the proto. I would not change it yet this information, but if there is anything we need to fix I would make a new PR after this will be merged, otherwise it will be difficult to see what are the changes / fixes.