feat(opentelemetry-exporter-jaeger): http sender #965

leonardodalcin · 2020-04-13T17:49:56Z

Which problem is this PR solving?

Enables the usage of jaeger-client-node HTTPSender in order to send spans over http.

The behavior implemented complies with the following jaeger-client-node documentation statement:

By default, the client sends traces via UDP to the agent at localhost:6832. 
Use JAEGER_AGENT_HOST and JAEGER_AGENT_PORT to send UDP traces to a different host:port. 
If JAEGER_ENDPOINT is set, the client sends traces to the endpoint via HTTP, making the JAEGER_AGENT_HOST and JAEGER_AGENT_PORT unused. 
If JAEGER_ENDPOINT is secured, HTTP basic authentication can be performed by setting the JAEGER_USER and JAEGER_PASSWORD environment variables.

Extracted from: https://github.com/jaegertracing/jaeger-client-node#environment-variables

Short description of the changes

1. packages/opentelemetry-exporter-jaeger/src/jaeger.ts:

Populates the config fields endpoint, username and password at JaegerExporter contruction . code
Checks if config.endpoint is setten and instanciates this._sender as a HTTPSender and reffers original documentation in commentaries. code

2. packages/opentelemetry-exporter-jaeger/src/types.ts:

Adds the properties endpoint, username and password to ExporterConfig interface . code
Declares HTTPSender as a jaeger type. code

codecov-io · 2020-04-13T18:31:00Z

Codecov Report

Merging #965 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #965      +/-   ##
==========================================
+ Coverage   92.31%   92.39%   +0.08%     
==========================================
  Files         117      118       +1     
  Lines        3396     3407      +11     
  Branches      689      694       +5     
==========================================
+ Hits         3135     3148      +13     
+ Misses        261      259       -2

Impacted Files	Coverage Δ
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts	`94.82% <100.00%> (+5.03%)`	⬆️
...ackages/opentelemetry-exporter-jaeger/src/types.ts	`100.00% <100.00%> (ø)`
...ackages/opentelemetry-exporter-jaeger/src/utils.ts	`100.00% <100.00%> (ø)`

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

vmarchaud · 2020-04-20T13:59:11Z

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

@@ -38,8 +39,21 @@ export class JaegerExporter implements SpanExporter {
      typeof config.flushTimeout === 'number' ? config.flushTimeout : 2000;

    config.host = config.host || process.env.JAEGER_AGENT_HOST;
+    config.endpoint = config.endpoint || process.env.JAEGER_ENDPOINT;
+    config.username = config.username || process.env.JAEGER_USER;
+    config.password = config.password || process.env.JAEGER_PASSWORD;


I woud prefer to verify that one of username/password or endpoint is defined and valid to avoid runtime errors.

Done in 7e64566

With your commit, i think it's not possible to only use JAEGER_AGENT_ENDPOINT env var since you check for config.endpoint which should be undefined in this case. Is that normal ?

If process.env.JAEGER_ENDPOINT is defined, then config.endpoint will also be. I don't think i understood what you meant.

packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts

vmarchaud

lgtm, only need the tests to pass

mayurkale22 · 2020-05-27T05:44:07Z

Any release date for this?

Sorry for the delay in response. We will include this change in 0.8.3 release. Let us know if you're blocked on this, we can cut the release sometime this week.

romilpunetha · 2020-05-27T05:48:18Z

Any release date for this?

Sorry for the delay in response. We will include this change in 0.8.3 release. Let us know if you're blocked on this, we can cut the release sometime this week.

Not a blocker. I was looking for a standard way to do this across node and java services. Java auto instrumentation sends it directly to the collector. I couldn't find a way to make use of the agent there, whereas nodejs agent would only send to the agent.

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

dyladan · 2020-06-03T16:53:03Z

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

+    // to the endpoint via HTTP, making the JAEGER_AGENT_HOST and JAEGER_AGENT_PORT unused. If JAEGER_ENDPOINT is secured,
+    // HTTP basic authentication can be performed by setting the JAEGER_USER and JAEGER_PASSWORD environment variables.
+    if (localConfig.endpoint) {
+      localConfig.endpoint = localConfig.endpoint || process.env.JAEGER_ENDPOINT;


this will never use the env

This should fix it: 8d51800

obecny

lgtm

mayurkale22 · 2020-06-08T21:24:31Z

@dyladan please take a look when you get a chance.

@leonardodalcin Please, resolve the conflicts.

linux-foundation-easycla · 2020-06-09T12:34:33Z

The committers are authorized under a signed CLA.

✅ Leonardo Dalcin (3942759, 0408488, 5de0fba, 256eb0e, cf5fe82, 2ef75b5, a2ce058, 0e1abfe, 7e64566, 0a15276, 5620e16, 90f919e, b595b54, 38dc850, 0d8b202, 2c75792, a528a1e, d284387, 8d51800, abcfcc7, 9398254)
✅ Mayur Kale (0235711)

dyladan · 2020-06-10T15:58:24Z

@leonardodalcin looks like this is ready to merge once conflicts are resolved

…ry#965)

feat(opentelemetry-exporter-jaeger): http sender

3942759

leonardodalcin requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners April 13, 2020 17:49

fix: linter

0408488

dyladan reviewed Apr 13, 2020

View reviewed changes

packages/opentelemetry-exporter-jaeger/src/jaeger.ts Outdated Show resolved Hide resolved

leonardodalcin added 6 commits April 14, 2020 10:40

Merge branch 'master' into http-sender

5de0fba

fix(opentelemetry-exporter-jaeger): adds header to avoid infinity loop

256eb0e

Merge branch 'master' into http-sender

cf5fe82

test(opentelemetry-exporter-jaeger): verify http sender usage

2ef75b5

Merge remote-tracking branch 'origin/http-sender' into http-sender

a2ce058

Merge branch 'master' into http-sender

0e1abfe

leonardodalcin requested a review from dyladan April 18, 2020 22:18

vmarchaud reviewed Apr 20, 2020

View reviewed changes

refactor(opentelemetry-exporter-jaeger): checks if endpoint is setten

7e64566

yurishkuro reviewed Apr 23, 2020

View reviewed changes

packages/opentelemetry-exporter-jaeger/src/jaeger.ts Show resolved Hide resolved

yurishkuro reviewed Apr 23, 2020

View reviewed changes

packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts Outdated Show resolved Hide resolved

This was referenced Apr 26, 2020

bug(http-plugin): x-opentelemetry-outgoing-request isn't stripped before sending the request #983

Closed

fix(http-plugin): strip otel custom http header #983 #984

Merged

leonardodalcin added 4 commits May 8, 2020 16:23

Merge branch 'master' into http-sender

0a15276

feat(opentelemetry-exporter-jaeger): adds nock as dev dependency

5620e16

test(opentelemetry-exporter-jaeger): adds tests to check req header

90f919e

Merge remote-tracking branch 'origin/http-sender' into http-sender

b595b54

vmarchaud approved these changes May 8, 2020

View reviewed changes

Merge branch 'master' into http-sender

0235711

mayurkale22 requested a review from mwear as a code owner May 27, 2020 05:40

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

mayurkale22 requested a review from yurishkuro May 27, 2020 05:50

Merge branch 'master' into http-sender

0d8b202

obecny reviewed May 27, 2020

View reviewed changes

packages/opentelemetry-exporter-jaeger/src/jaeger.ts Outdated Show resolved Hide resolved

packages/opentelemetry-exporter-jaeger/src/jaeger.ts Outdated Show resolved Hide resolved

dyladan requested changes Jun 2, 2020

View reviewed changes

packages/opentelemetry-exporter-jaeger/src/jaeger.ts Outdated Show resolved Hide resolved

leonardodalcin added 2 commits June 3, 2020 13:49

refactor(opentelemetry-exporter-jaeger): removes config parameter change

2c75792

Merge branch 'master' into http-sender

a528a1e

dyladan reviewed Jun 3, 2020

View reviewed changes

leonardodalcin added 3 commits June 3, 2020 14:02

fix: linter

d284387

fix: env var was never called

8d51800

Merge remote-tracking branch 'origin/http-sender' into http-sender

abcfcc7

leonardodalcin requested review from dyladan and obecny June 3, 2020 17:08

obecny approved these changes Jun 8, 2020

View reviewed changes

markwolff approved these changes Jun 8, 2020

View reviewed changes

Merge branch 'master' into http-sender

9398254

leonardodalcin requested a review from naseemkullah as a code owner June 9, 2020 12:34

dyladan approved these changes Jun 9, 2020

View reviewed changes

dyladan and others added 2 commits June 10, 2020 11:59

Merge branch 'master' into http-sender

69d3f13

Merge branch 'master' into http-sender

2e94c67

mayurkale22 merged commit 21f0862 into open-telemetry:master Jun 10, 2020

leonardodalcin deleted the http-sender branch December 1, 2021 14:01

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

fix: remove forcing RUN_MSSQL_TESTS to be true in tests (open-telemet…

bcbdeb7

…ry#965)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opentelemetry-exporter-jaeger): http sender #965

feat(opentelemetry-exporter-jaeger): http sender #965

leonardodalcin commented Apr 13, 2020 •

edited

Loading

codecov-io commented Apr 13, 2020 •

edited by codecov bot

Loading

vmarchaud Apr 20, 2020

leonardodalcin Apr 20, 2020

vmarchaud Apr 21, 2020

leonardodalcin Apr 23, 2020

vmarchaud left a comment

mayurkale22 commented May 27, 2020

romilpunetha commented May 27, 2020

dyladan Jun 3, 2020

leonardodalcin Jun 3, 2020

obecny left a comment

mayurkale22 commented Jun 8, 2020

linux-foundation-easycla bot commented Jun 9, 2020 •

edited

Loading

dyladan commented Jun 10, 2020

feat(opentelemetry-exporter-jaeger): http sender #965

feat(opentelemetry-exporter-jaeger): http sender #965

Conversation

leonardodalcin commented Apr 13, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov-io commented Apr 13, 2020 • edited by codecov bot Loading

Codecov Report

vmarchaud Apr 20, 2020

Choose a reason for hiding this comment

leonardodalcin Apr 20, 2020

Choose a reason for hiding this comment

vmarchaud Apr 21, 2020

Choose a reason for hiding this comment

leonardodalcin Apr 23, 2020

Choose a reason for hiding this comment

vmarchaud left a comment

Choose a reason for hiding this comment

mayurkale22 commented May 27, 2020

romilpunetha commented May 27, 2020

dyladan Jun 3, 2020

Choose a reason for hiding this comment

leonardodalcin Jun 3, 2020

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

mayurkale22 commented Jun 8, 2020

linux-foundation-easycla bot commented Jun 9, 2020 • edited Loading

dyladan commented Jun 10, 2020

leonardodalcin commented Apr 13, 2020 •

edited

Loading

codecov-io commented Apr 13, 2020 •

edited by codecov bot

Loading

linux-foundation-easycla bot commented Jun 9, 2020 •

edited

Loading