Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix configuring an ipv6 agent hostname #2486

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Conversation

rochdev
Copy link
Member

@rochdev rochdev commented Nov 1, 2022

What does this PR do?

Fix configuring an IPv6 agent hostname.

Motivation

This was supported when passed as a complete URL but not when passing the IP as the hostname.

Fixes #2449

Additional Notes

I didn't test span stats since there isn't an existing test for exporting that I could find.

@rochdev rochdev added bug Something isn't working core semver-patch labels Nov 1, 2022
@rochdev rochdev requested a review from a team as a code owner November 1, 2022 20:55
@pr-commenter
Copy link

pr-commenter bot commented Nov 1, 2022

Benchmarks

Parameters

Baseline Candidate
config baseline candidate
git_branch master fix-ipv6-hostname-config
git_commit_sha f0424fc 4acdc8c
See matching parameters
Baseline Candidate
name profiler profiler
nodeVersion 14.20.1 14.20.1
variant control control

Summary

Found 5 performance improvements and 38 performance regressions! Performance is the same for 1007 cases.

scenario Δ mean cpu_system_time Δ mean cpu_usage_percentage Δ mean cpu_user_time Δ mean execution_time Δ mean max_event_loop_delay Δ mean max_gc_pause Δ mean max_rss_usage
scenario:profiler-control-14 same same unsure
[+0.933ms; +29.887ms] or [+0.930%; +29.779%]
worse
[+11.619ms; +14.189ms] or [+9.847%; +12.025%]
same worse
[+0.264ms; +0.377ms] or [+23.299%; +33.255%]
worse
[+1.642KB; +2.083KB] or [+3.982%; +5.053%]
scenario:profiler-with-cpu-profiler-14 same same worse
[+2.901ms; +23.930ms] or [+2.254%; +18.590%]
worse
[+13.203ms; +19.091ms] or [+8.824%; +12.758%]
same worse
[+0.278ms; +0.399ms] or [+24.490%; +35.196%]
worse
[+1.324KB; +1.737KB] or [+3.054%; +4.006%]
scenario:profiler-with-space-profiler-14 same unsure
[+0.419%; +1.812%]
unsure
[+0.917ms; +24.396ms] or [+0.892%; +23.710%]
worse
[+10.598ms; +13.234ms] or [+8.746%; +10.921%]
better
[-1.629ms; -0.116ms] or [-26.058%; -1.851%]
worse
[+0.358ms; +0.415ms] or [+33.287%; +38.598%]
worse
[+1.686KB; +1.930KB] or [+4.076%; +4.668%]
scenario:profiler-with-all-profilers-14 same same same worse
[+10.232ms; +16.997ms] or [+6.715%; +11.155%]
same worse
[+0.299ms; +0.434ms] or [+27.069%; +39.276%]
worse
[+1.461KB; +1.775KB] or [+3.367%; +4.091%]
scenario:plugin-bluebird-with-tracer-14 worse
[+1.238ms; +4.120ms] or [+5.302%; +17.653%]
same unsure
[-3.655ms; -0.779ms] or [-1.678%; -0.357%]
unsure
[+0.118ms; +0.565ms] or [+0.049%; +0.233%]
unsure
[-0.041KB; -0.008KB] or [-0.079%; -0.016%]
scenario:net-with-tracer-14 same same same same better
[-2.163KB; -1.581KB] or [-3.880%; -2.836%]
scenario:startup-with-tracer-everything-14 same same unsure
[-0.036s; -0.010s] or [-2.769%; -0.739%]
better
[-0.038s; -0.030s] or [-2.543%; -1.984%]
unsure
[-1.838KB; -1.262KB] or [-1.595%; -1.095%]
scenario:profiler-control-16 same same same worse
[+9.822ms; +12.115ms] or [+8.032%; +9.908%]
same same worse
[+1.095KB; +1.688KB] or [+2.483%; +3.827%]
scenario:profiler-with-cpu-profiler-16 same same worse
[+9.383ms; +25.797ms] or [+7.662%; +21.066%]
worse
[+10.629ms; +14.211ms] or [+7.417%; +9.917%]
worse
[+1.293ms; +2.265ms] or [+8.080%; +14.152%]
same worse
[+1.447KB; +1.647KB] or [+3.116%; +3.548%]
scenario:profiler-with-space-profiler-16 same same worse
[+3.964ms; +24.531ms] or [+3.642%; +22.535%]
worse
[+8.865ms; +13.392ms] or [+7.058%; +10.662%]
same same worse
[+1.060KB; +1.665KB] or [+2.388%; +3.753%]
scenario:profiler-with-all-profilers-16 same same same worse
[+10.252ms; +14.026ms] or [+7.017%; +9.600%]
worse
[+1.442ms; +2.359ms] or [+8.942%; +14.630%]
same worse
[+1.493KB; +2.032KB] or [+3.223%; +4.388%]
scenario:startup-control-everything-16 unsure
[-18.000ms; -2.137ms] or [-11.706%; -1.390%]
same unsure
[-0.035s; -0.009s] or [-3.115%; -0.821%]
better
[-0.043s; -0.022s] or [-3.326%; -1.680%]
same
scenario:exporting-pipeline-0.4_with_stats-16 same same unsure
[+12.418ms; +45.675ms] or [+1.273%; +4.681%]
unsure
[+0.014s; +0.047s] or [+1.336%; +4.529%]
better
[-8.828KB; -3.294KB] or [-9.182%; -3.427%]
scenario:plugin-dns-with-tracer-16 same unsure
[+0.217%; +0.665%]
worse
[+8.648ms; +25.949ms] or [+2.182%; +6.548%]
same same
scenario:profiler-control-18 same same same worse
[+11.823ms; +12.795ms] or [+8.958%; +9.695%]
same unsure
[-0.040ms; -0.003ms] or [-2.970%; -0.248%]
worse
[+1.131KB; +1.485KB] or [+2.236%; +2.938%]
scenario:profiler-with-cpu-profiler-18 same same same worse
[+12.197ms; +14.602ms] or [+7.880%; +9.435%]
worse
[+1.459ms; +2.519ms] or [+8.379%; +14.471%]
same worse
[+1.467KB; +1.744KB] or [+2.771%; +3.296%]
scenario:profiler-with-space-profiler-18 same unsure
[-1.238%; -0.107%]
same worse
[+11.447ms; +12.800ms] or [+8.471%; +9.473%]
same same worse
[+1.105KB; +1.439KB] or [+2.171%; +2.827%]
scenario:profiler-with-all-profilers-18 same same unsure
[+1.257ms; +23.587ms] or [+0.947%; +17.761%]
worse
[+11.956ms; +14.411ms] or [+7.605%; +9.167%]
worse
[+1.715ms; +2.440ms] or [+9.902%; +14.086%]
same worse
[+1.503KB; +1.822KB] or [+2.835%; +3.435%]
scenario:async_hooks-init-only-18 worse
[+1.767ms; +17.403ms] or [+1.653%; +16.281%]
same unsure
[-20.881ms; -1.891ms] or [-2.261%; -0.205%]
same unsure
[-3.116ms; -0.194ms] or [-1.406%; -0.088%]
same same

@rochdev rochdev merged commit 6e94eee into master Nov 1, 2022
@rochdev rochdev deleted the fix-ipv6-hostname-config branch November 1, 2022 21:05
rochdev added a commit that referenced this pull request Nov 1, 2022
rochdev added a commit that referenced this pull request Nov 1, 2022
rochdev added a commit that referenced this pull request Nov 1, 2022
rochdev added a commit that referenced this pull request Nov 1, 2022
amymin00 pushed a commit that referenced this pull request Nov 2, 2022
@clhuang
Copy link

clhuang commented Dec 19, 2022

This appears to still be broken, but with a slightly different error; in the logs I see:

Request to the agent: {"path":"/v0.4/traces","method":"PUT","headers":{"Content-Type":"application/msgpack","Datadog-Meta-Tracer-Version":"3.9.3","X-Datadog-Trace-Count":"5","Datadog-Meta-Lang":"nodejs","Datadog-Meta-Lang-Version":"v16.16.0","Datadog-Meta-Lang-Interpreter":"v8"},"url":"http://[2602:fb33::348d]:8126/"}
Error: getaddrinfo ENOTFOUND [2602:fb33::348d]
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)
    at GetAddrInfoReqWrap.callbackTrampoline (node:internal/async_hooks:130:17) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: '[2602:fb33::348d]'
}

It seems to be because we are directly passing the fields from the URL object to the http(s) request field, which is not valid for IPv6 addresses (see nodejs/node#39738); urlToHttpOptions should be used to do the conversion

@clhuang
Copy link

clhuang commented Dec 19, 2022

alternatively, you could just do options.hostname = url.hostname.startsWith('[') ? url.hostname.slice(1, -1) : url.hostname in https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/src/exporters/common/request.js#L34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core semver-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DD_AGENT_HOST is not used properly when ipv6 address
3 participants