fix(instrumentation-undici): fix a possible crash if the request path is a full URL

[trentm]

Closes: #2471

[trentm]

test failure without the fix

Here is what the added test would have looked like failing if the fix wasn't included:

$ npm test
...
  UndiciInstrumentation `undici` tests
    disable()
      ✔ should not create spans when disabled
    enable()
      ✔ should ignore requests based on the result of ignoreRequestHook
      ✔ should create valid spans for different request methods
      ✔ should create valid spans for "request" method
      ✔ should create valid spans for "fetch" method
      ✔ should create valid spans for "stream" method
      ✔ should create valid spans for "dispatch" method
      ✔ should create valid spans even if the configuration hooks fail
      ✔ should not create spans without parent if required in configuration
      ✔ should not create spans with INVALID_SPAN_CONTEXT parent if required in configuration
      ✔ should create spans with parent if required in configuration
      ✔ should capture errors while doing request
      ✔ should capture error if undici request is aborted
      ✔ should not report an user-agent if it was not defined
      1) should create valid span if request.path is a full URL


  24 passing (189ms)
  1 failing

  1) UndiciInstrumentation `undici` tests
       enable()
         should create valid span if request.path is a full URL:
     Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
      at new NodeError (node:internal/errors:405:5)
      at new URL (node:internal/url:676:13)
      at UndiciInstrumentation.onRequestCreated (src/undici.ts:82:34)
      at Channel.publish (node:diagnostics_channel:141:9)
      at new Request (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/core/request.js:190:23)
      at Client.[dispatch] (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/dispatcher/client.js:304:21)
      at Intercept (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/interceptor/redirect-interceptor.js:11:16)
      at Client.[Intercepted Dispatch] (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/dispatcher/dispatcher-base.js:158:12)
      at Client.dispatch (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/dispatcher/dispatcher-base.js:179:40)
      at Client.request (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/api/api-request.js:170:10)
      at /Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/api/api-request.js:163:15
      at new Promise (<anonymous>)
      at Client.request (/Users/trentm/tm/opentelemetry-js-contrib2/node_modules/undici/lib/api/api-request.js:162:12)
      at Context.<anonymous> (test/undici.test.ts:842:32)
      at processImmediate (node:internal/timers:476:21)
      at process.topLevelDomainCallback (node:domain:161:15)
      at process.callbackTrampoline (node:internal/async_hooks:126:24)

[trentm]

diag.warn if the new URL fails again

If there is some other case where the new URL(...) throws, then the diag.warn would look like the following. This is using the repro script and setup from #2471 (comment)

% node -r '@opentelemetry/auto-instrumentations-node/register' use-solr.js
OTEL_LOGS_EXPORTER is empty. Using default otlp exporter.
OTEL_TRACES_EXPORTER is empty. Using default otlp exporter.
OpenTelemetry automatic instrumentation started successfully
@opentelemetry/instrumentation-undici could not determine url.full: TypeError [ERR_INVALID_URL]: Invalid URL
    at new NodeError (node:internal/errors:405:5)
    at new URL (node:internal/url:676:13)
    at UndiciInstrumentation.onRequestCreated (/Users/trentm/tm/opentelemetry-js-contrib2/plugins/node/instrumentation-undici/build/src/undici.js:128:26)
    at Channel.publish (node:diagnostics_channel:141:9)
    at new Request (/Users/trentm/tmp/asdf.20241105T113812/node_modules/undici/lib/core/request.js:141:23)
    at Client.dispatch (/Users/trentm/tmp/asdf.20241105T113812/node_modules/undici/lib/client.js:294:23)
    at Client.request (/Users/trentm/tmp/asdf.20241105T113812/node_modules/undici/lib/api/api-request.js:154:10)
    at /Users/trentm/tmp/asdf.20241105T113812/node_modules/undici/lib/api/api-request.js:147:15
    at new Promise (<anonymous>)
    at Client.request (/Users/trentm/tmp/asdf.20241105T113812/node_modules/undici/lib/api/api-request.js:146:12) {
  input: 'http://127.0.0.1:8983http://127.0.0.1:8983/solr/gettingstarted/admin/ping?wt=json',
  code: 'ERR_INVALID_URL'
}
solr response:  {
  responseHeader: {
    zkConnected: null,
    status: 0,
    QTime: 3,
    params: {
      q: '{!lucene}*:*',
      distrib: 'false',
      df: '_text_',
      rows: '10',
      wt: 'json',
      echoParams: 'all',
      rid: '-9'
    }
  },
  status: 'OK'
}

This isn't perfect: If there is a user case where the instrumentation starts diag.warn'ing here, there will be a diag.warn for every such request. It would be nice to have a diag.warnOnce or similar mechanism. I didn't see any other examples in the core or contrib repos doing this. Perhaps we could cross that road when we come to it.

[trentm]

Note that I haven't added a test case for this diag.warn case. I don't immediately know of way to (a) make an undici request that passes undici's guards but (b) fails this one.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.51%. Comparing base (0309cae) to head (ef5ac34).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
plugins/node/instrumentation-undici/src/undici.ts	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2518      +/-   ##
==========================================
- Coverage   90.77%   90.51%   -0.27%     
==========================================
  Files         169      164       -5     
  Lines        8015     7707     -308     
  Branches     1632     1591      -41     
==========================================
- Hits         7276     6976     -300     
+ Misses        739      731       -8     

Files with missing lines	Coverage Δ
plugins/node/instrumentation-undici/src/undici.ts	92.74% <50.00%> (-0.94%)	⬇️

... and 6 files with indirect coverage changes

[david-luna]

It would be nice to have a diag.warnOnce or similar mechanism.

did you mean a unique warn per error type or maybe one warn per bad URL? IMO second would be preferable because in this specific case bad URLs may come from different code paths

Note that I haven't added a test case for this diag.warn case. I don't immediately know of way to (a) make an undici request that passes undici's guards but (b) fails this one.

I think that this might be possible with a non supported version of undici (the one is used by solr-node-client). IMO do a setup to test an unsupported version is too much

[pichlermarc]

Thanks for working on this 🙂