feat: exporter collector TLS option #1063

mzahor · 2020-05-15T15:46:37Z

Resolves this: #1062

codecov-io · 2020-05-15T15:51:58Z

Codecov Report

Merging #1063 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1063   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files         213      213           
  Lines        8950     8951    +1     
  Branches      806      806           
=======================================
+ Hits         8514     8515    +1     
  Misses        436      436

Impacted Files	Coverage Δ
...emetry-exporter-collector/src/CollectorExporter.ts	`90.47% <100.00%> (+0.23%)`	⬆️

dyladan · 2020-05-15T15:54:41Z

Certs exist only for testing purposes yes? If so they should be in the test directory.

mzahor · 2020-05-18T08:42:39Z

Certs exist only for testing purposes yes? If so they should be in the test directory.

Good point, fixed. Let me know if you have any other comments.

vmarchaud

lgtm

dyladan · 2020-05-18T13:41:52Z

packages/opentelemetry-exporter-collector/test/certs/regenerate.sh

@@ -0,0 +1,11 @@
+rm ca.crt ca.key client.crt client.csr client.key server.crt server.csr server.key


Can you please add some comments or documentation on what this file does, when it should be run, why it is here?

sure, would this document be a good place for it?

Don't think so. That file is for general contribution guidelines and things that apply to the whole repository, not a specific package. I would just add it as comments to the top of this file, and/or add a note in the collector exporter readme.

Got it, thanks.

…to feat/otlp-tls

dyladan · 2020-05-18T14:44:15Z

packages/opentelemetry-exporter-collector/README.md

@@ -49,6 +49,32 @@ provider.register();

 ```

+By default, plaintext connection is used. In order to use TLS, provide `credentials` option like so:


Please add a note similar to "to see how to generate credentials, you can refer to the script used to generate certificates for tests [here](link to certs sh)."

mayurkale22 · 2020-05-18T19:54:59Z

@obecny would like to get your eyes on this.

obecny

I think this needs to be refactored into specific platforms

obecny · 2020-05-18T20:54:41Z

packages/opentelemetry-exporter-collector/src/CollectorExporter.ts

 import { ExportResult, NoopLogger } from '@opentelemetry/core';
 import { ReadableSpan, SpanExporter } from '@opentelemetry/tracing';
-import { Attributes, Logger } from '@opentelemetry/api';
+import * as grpc from 'grpc';


CollectorExporter is used by web and node. grpc is not part of web so this won't be working fine and also credentials are not needed in web so far.
I think ExporterCollector should be treated as base class and not be exported (outside - only internally). Then inside folder platform inside node and browser you should create a new files ExporterCollector. The web can simply return the "base" ExporterCollector but the node you can then simply extend the base with all needed functionality that you added here. Then export the extended class correctly

I understand the credentials will only work in node, but it won't break web right?

it will break as grpc is a node package, cannot be included into web

unless we just let credentials to exists in both, but then explain that you should not use it which might be misleading why. By splitting this it will be easier to add stuff I think

It is type only and is compiled away. In recent versions of ts there is even a way to enforce this by doing import type { ChannelCredentials } from 'grpc';

I'm affraid that once we allow having anything from grpc here it is like asking for more trouble in future - who will remember then that we should not do that. Splitting this now will save us troubles later and it will be easier to maintain it too.

@obecny I've refactored it as you suggested. Please check if web/node exports are done correctly, I'm not sure I understand the initial idea with separate exports for each env. Let me know if you have any other comments.

Also if you wanna do any changes yourself - feel free to push.

obecny · 2020-05-18T20:56:41Z

packages/opentelemetry-exporter-collector/README.md

@@ -49,6 +49,34 @@ provider.register();

 ```

+By default, plaintext connection is used. In order to use TLS, provide `credentials` option like so:
+```js


please mark this is for node only, not browser

codecov-commenter · 2020-05-19T09:46:03Z

Codecov Report

Merging #1063 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
- Coverage   95.17%   95.17%   -0.01%     
==========================================
  Files         216      216              
  Lines        9063     9062       -1     
  Branches      819      819              
==========================================
- Hits         8626     8625       -1     
  Misses        437      437

Impacted Files	Coverage Δ
...ry-exporter-collector/src/CollectorExporterBase.ts	`90.00% <100.00%> (ø)`
.../opentelemetry-exporter-collector/src/transform.ts	`95.71% <100.00%> (ø)`

…to feat/otlp-tls

obecny

the changes lgtm, I checked if it works fine and there was just one bug with web version - added comment.

There is one thing which can be now changed / refactored. This is with regards to platform/node/CollectorExporter.ts line 32
traceServiceClients = ... it was needed before to have reference to Collector, now you could simply use this as you have a Collector instance in place.

FYI if you want to test collector in web

update lerna.json and add

    "examples/collector-exporter-node",
    "examples/tracer-web",

into packages section - this is just temporary to be able to bootstrap and debug examples (don't commit this)
2. npm i from root folder
3. npm run watch from packages/opentelemetry-exporter-collector
4. npm run start from examples/tracer-web
5. npm run docker:start from examples/collector-exporter-node
6. Open page at http://localhost:8090/user-interaction/ and click some button to generate traces
7. Open page at http://localhost:9411/zipkin/ to check traces

packages/opentelemetry-exporter-collector/src/index.ts

packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporter.ts

mzahor · 2020-05-19T14:13:06Z

@obecny Thanks a lot for the detailed feedback! Fixed all the comments and checked as you suggested with web example - seems to work fine. Let me know if you have any other comments.

obecny

thx for all your work, looks gr8 :)

* fix(sendMessageBatch): add sqs test * fix(sendMessageBatch): remove unused parameter * fix(fix comment): fix comment

feat: exporter collector TLS option

4c5c029

mzahor requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners May 15, 2020 15:46

dyladan linked an issue May 15, 2020 that may be closed by this pull request

feat: TLS support for collector exporter #1062

Closed

mzahor added 3 commits May 18, 2020 11:18

refactor: move certs to tests folder

1f5db6d

Merge branch 'master' into feat/otlp-tls

0451261

refactor: code style

622ded7

vmarchaud approved these changes May 18, 2020

View reviewed changes

dyladan reviewed May 18, 2020

View reviewed changes

mzahor added 3 commits May 18, 2020 16:55

Merge branch 'master' into feat/otlp-tls

cc3c75a

doc: add TLS docs

b059956

Merge branch 'feat/otlp-tls' of github.com:mzahor/opentelemetry-js in…

40567dc

…to feat/otlp-tls

dyladan reviewed May 18, 2020

View reviewed changes

doc: add reference to TLS cert gen script

764a555

dyladan approved these changes May 18, 2020

View reviewed changes

dyladan mentioned this pull request May 18, 2020

feat: add OTEL_SAMPLING_PROBABILITY env var #1069

Merged

mayurkale22 approved these changes May 18, 2020

View reviewed changes

mayurkale22 added the enhancement New feature or request label May 18, 2020

obecny suggested changes May 18, 2020

View reviewed changes

obecny reviewed May 18, 2020

View reviewed changes

mzahor added 5 commits May 19, 2020 12:12

refactor: use separate impl for node and browser

9d1ae0b

fix: unit tests

b2eae13

style: fix codestyle

182e6c7

fix: compilation

61e4916

doc: emphasize that TLS is Node-specific

7a99848

mzahor added 7 commits May 19, 2020 12:46

Merge branch 'master' into feat/otlp-tls

9c3158b

fix: doc comment

a2e7acf

Merge branch 'feat/otlp-tls' of github.com:mzahor/opentelemetry-js in…

8db7d91

…to feat/otlp-tls

Merge branch 'feat/otlp-tls' of github.com:mzahor/opentelemetry-js in…

bae411c

…to feat/otlp-tls

Merge branch 'feat/otlp-tls' of github.com:mzahor/opentelemetry-js in…

f2b5bc1

…to feat/otlp-tls

fix: exports

1deecaf

style: fix linting issues

1e31648

obecny reviewed May 19, 2020

View reviewed changes

refactor: remove weakmap

74eab02

obecny approved these changes May 19, 2020

View reviewed changes

obecny mentioned this pull request May 19, 2020

fix(exporter-collector): default endpoint #1070

Closed

dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label May 19, 2020

Merge branch 'master' into feat/otlp-tls

231d228

mayurkale22 merged commit e912842 into open-telemetry:master May 19, 2020

mzahor deleted the feat/otlp-tls branch May 20, 2020 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: exporter collector TLS option #1063

feat: exporter collector TLS option #1063

mzahor commented May 15, 2020

codecov-io commented May 15, 2020 •

edited

Loading

dyladan commented May 15, 2020

mzahor commented May 18, 2020

vmarchaud left a comment

dyladan May 18, 2020

mzahor May 18, 2020

dyladan May 18, 2020

mzahor May 18, 2020

dyladan May 18, 2020

mayurkale22 commented May 18, 2020

obecny left a comment

obecny May 18, 2020 •

edited

Loading

dyladan May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

dyladan May 18, 2020

obecny May 18, 2020

mzahor May 19, 2020

mzahor May 19, 2020

obecny May 18, 2020

codecov-commenter commented May 19, 2020 •

edited

Loading

obecny left a comment •

edited

Loading

mzahor commented May 19, 2020

obecny left a comment

		@@ -0,0 +1,11 @@
		rm ca.crt ca.key client.crt client.csr client.key server.crt server.csr server.key

		@@ -49,6 +49,32 @@ provider.register();

		```

		By default, plaintext connection is used. In order to use TLS, provide `credentials` option like so:

feat: exporter collector TLS option #1063

feat: exporter collector TLS option #1063

Conversation

mzahor commented May 15, 2020

codecov-io commented May 15, 2020 • edited Loading

Codecov Report

dyladan commented May 15, 2020

mzahor commented May 18, 2020

vmarchaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented May 18, 2020

obecny left a comment

Choose a reason for hiding this comment

obecny May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2020 • edited Loading

Codecov Report

obecny left a comment • edited Loading

Choose a reason for hiding this comment

mzahor commented May 19, 2020

obecny left a comment

Choose a reason for hiding this comment

codecov-io commented May 15, 2020 •

edited

Loading

obecny May 18, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading

obecny left a comment •

edited

Loading