udp6 support by doochik · Pull Request #396 · jaegertracing/jaeger-client-node

doochik · 2019-09-13T15:36:13Z

Which problem is this PR solving?

Jaegler client supports udp4 socket only.

Short description of the changes

I've added agentSocketType to reporter config to add support for IPv6 udp sockets.
https://nodejs.org/dist/latest-v10.x/docs/api/dgram.html#dgram_dgram_createsocket_type_callback

Example

const jaegerConfig = {
    reporter: {
        agentHost: 'jaeger-agent.query.consul',
        agentPort: 5775,
        agentSocketType: 'udp6',
    },
    ....
};

yurishkuro

Love the PR.

Q: what would be the reasons for someone to prefer v6 over v4? Does it matter on the localhost for UDP? If there are good reasons, perhaps mention them in the readme when describing the new env var, as recommendation to the user.

src/configuration_env.js

test/udp_sender.js

doochik · 2019-09-13T18:29:18Z

Love the PR.

Q: what would be the reasons for someone to prefer v6 over v4? Does it matter on the localhost for UDP? If there are good reasons, perhaps mention them in the readme when describing the new env var, as recommendation to the user.

I have external (not localhost) ipv6-only jaeger-agent. So I can't use udp4.

yurishkuro · 2019-09-13T19:44:03Z

I have external (not localhost) ipv6-only jaeger-agent. So I can't use udp4.

We don't recommend non-local agents, the agent wasn't designed for this. For sending spans to a remote received it's better to use TCP-based protocols, e.g. HTTP directly to the collector.

travis-ci/travis-ci#8361 (comment) travis-ci/travis-ci#8891 Signed-off-by: Aleksei Androsov <[email protected]>

Support to send udp over IPv6. https://nodejs.org/dist/latest-v10.x/docs/api/dgram.html#dgram_dgram_createsocket_type_callback Signed-off-by: Aleksei Androsov <[email protected]>

codecov · 2019-09-14T09:47:03Z

Codecov Report

Merging #396 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
+ Coverage   98.65%   98.65%   +<.01%     
==========================================
  Files          50       50              
  Lines        2003     2010       +7     
  Branches      374      377       +3     
==========================================
+ Hits         1976     1983       +7     
  Misses         27       27

Impacted Files	Coverage Δ
src/configuration.js	`98.97% <100%> (+0.02%)`	⬆️
src/reporters/udp_sender.js	`98.73% <100%> (+0.03%)`	⬆️
src/configuration_env.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cbc7e9...8e0b003. Read the comment docs.

doochik · 2019-09-14T09:59:01Z

@yurishkuro I've fixed all of you comments

src/configuration.js

src/configuration_env.js

yurishkuro · 2019-09-14T16:48:11Z

test/udp_sender.js

-});
+
+    it('should gracefully handle errors emitted by socket.send', function(done) {
+      // EAI_AGAIN - nodejs received invalid DNS response. E.g. resolver doesn't support IPv6.


When would this error happen in the test?

This errors occurs when dns resolver doesn't support IPv6 or host doesn't have ipv6 resolver. I've got this error in travis for upd6 and foo.bar.xyz instead of ENOTFOUND.

For example,
on my local Mac I've got ENOTFOUND for this domain because I have dns resolver.
In travis I've got EAI_AGAIN

yurishkuro · 2019-09-14T16:49:03Z

test/udp_sender.js

+    it('should gracefully handle errors emitted by socket.send', function(done) {
+      // EAI_AGAIN - nodejs received invalid DNS response. E.g. resolver doesn't support IPv6.
+      // ENOTFOUND - nodejs received valid DNS response but domain not found
+      // ESRCH - nodejs v0.10 error


is that specifically for udp6? Since before the tests were passing with just ENOTFOUND

Yes, is specifically for upd6

doochik · 2019-09-15T14:00:11Z

Red tests because of this regression Raynos/error#19

deps: jaeger-client-node -> tchannel, thriftrw -> bufrw -> error

Add tests for configuration options Signed-off-by: Aleksei Androsov <[email protected]>

doochik · 2019-09-16T12:47:17Z

Tests are green

doochik · 2019-09-18T09:55:00Z

@yurishkuro could you release new version please?

`agentSocketType` is the new config option jaegertracing/jaeger-client-node#396 https://github.com/jaegertracing/jaeger-client-node/releases/tag/v3.17.0 `serviceName` at `PrometheusMetricsFactory` must be optional. https://github.com/jaegertracing/jaeger-client-node/blob/4c739a26d57c615fb4ac53eae921b9fd5ca53df2/src/metrics/prometheus.js#L57

doochik force-pushed the upd6-support branch from 768f94d to 0646645 Compare September 13, 2019 15:39

yurishkuro reviewed Sep 13, 2019

View reviewed changes

src/configuration_env.js Outdated Show resolved Hide resolved

test/udp_sender.js Outdated Show resolved Hide resolved

Enable IPv6 in travis

60a292f

travis-ci/travis-ci#8361 (comment) travis-ci/travis-ci#8891 Signed-off-by: Aleksei Androsov <[email protected]>

doochik force-pushed the upd6-support branch 2 times, most recently from bad4d71 to 43d5231 Compare September 14, 2019 09:37

udp6 support

da5f59b

Support to send udp over IPv6. https://nodejs.org/dist/latest-v10.x/docs/api/dgram.html#dgram_dgram_createsocket_type_callback Signed-off-by: Aleksei Androsov <[email protected]>

doochik force-pushed the upd6-support branch from 43d5231 to da5f59b Compare September 14, 2019 09:47

yurishkuro approved these changes Sep 14, 2019

View reviewed changes

yurishkuro reviewed Sep 14, 2019

View reviewed changes

doochik force-pushed the upd6-support branch 3 times, most recently from e8a8b69 to 2e2238d Compare September 15, 2019 13:58

udp6 support

8e0b003

Add tests for configuration options Signed-off-by: Aleksei Androsov <[email protected]>

doochik force-pushed the upd6-support branch from 2e2238d to 8e0b003 Compare September 15, 2019 15:51

yurishkuro merged commit 4a35dd1 into jaegertracing:master Sep 16, 2019

doochik deleted the upd6-support branch April 13, 2021 15:27

Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021

chore: fix typos and remove unwanted devDependency (jaegertracing#396)

9f82d10

Conversation

doochik commented Sep 13, 2019

Which problem is this PR solving?

Short description of the changes

Uh oh!

yurishkuro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

doochik commented Sep 13, 2019

Uh oh!

yurishkuro commented Sep 13, 2019

Uh oh!

codecov bot commented Sep 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

doochik commented Sep 14, 2019

Uh oh!

Uh oh!

Uh oh!

yurishkuro Sep 14, 2019

Choose a reason for hiding this comment

Uh oh!

doochik Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

doochik Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

yurishkuro Sep 14, 2019

Choose a reason for hiding this comment

Uh oh!

doochik Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

doochik commented Sep 15, 2019

Uh oh!

doochik commented Sep 16, 2019

Uh oh!

doochik commented Sep 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yurishkuro left a comment •

edited

Loading

codecov bot commented Sep 14, 2019 •

edited

Loading