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

instrumentation-undici can throw "[ERR_INVALID_URL]: Invalid URL" in 'undici:request:create' handler #2471

Closed
neo1parekh opened this issue Oct 10, 2024 · 10 comments · Fixed by #2518 or #2507
Assignees
Labels
bug Something isn't working pkg:instrumentation-undici

Comments

@neo1parekh
Copy link

Hi Folks,

I have been using solr-node-client for a long time, I have started using new package opentelemetry JAVASCRIPT packages.
"@opentelemetry/auto-instrumentations-node": "^0.50.0"
"@opentelemetry/exporter-trace-otlp-http": "^0.53.0"
"@opentelemetry/sdk-node": "^0.53.0"

under this opentelemetry-js-contrib package uses undici dependencies package.

which required unidici version greater than 5.2

reference link

But solr-client package required unidici version 4.11.1.

Can someone help how would I run both npm opentelementry and solr-node-client together or any other solution which can work out.

Any help would really appreciated.

@neo1parekh neo1parekh changed the title Support needed with solr-client dependancy package Help needed with package conflict. Oct 10, 2024
@pichlermarc
Copy link
Member

@neo1parekh the support statement on @opentelemetry/instrumentation-undici means that it'll only generate telemetry for undici@>=5.2, but you can still use older undici versions in the same app.

@pichlermarc
Copy link
Member

@neo1parekh the support statement on @opentelemetry/instrumentation-undici means that it'll only generate telemetry for undici@>=5.2, but you can still use older undici versions in the same app.

@neo1parekh does that answer the question? 🙂
Support for older versions would be a feature request.

@neo1parekh
Copy link
Author

neo1parekh commented Oct 18, 2024

Hi @pichlermarc sorry couldn't reply earlier, Thanks for reminding message.

In @opentelemetry/instrumentation-undici package.json undici version is 6.11.1 using and I want to start using opentelementry version, using older version might cause feature updation.

I understand to build for older version.
Is there any mid way to solve this.

@pichlermarc
Copy link
Member

@neo1parekh as said earlier this should still work. @opentelemetry/instrumentation-undici does not depend on undici it's a devDependency that won't be installed in your app if you install it.

Is there anything that I'm missing that leads you to believe that this wouldn't work?

@neo1parekh
Copy link
Author

neo1parekh commented Oct 18, 2024

@pichlermarc
Agreed and clear by your point.

Have question if you can help.
Following is package I have used
Package.json
"@opentelemetry/auto-instrumentations-node": "^0.50.2", "@opentelemetry/exporter-trace-otlp-http": "^0.53.0", "@opentelemetry/sdk-node": "^0.53.0", "solr-client": "^0.10.0-rc10",

by installing solr-client which install undici version 4.16.0. Which raise following error.

Uncaught TypeError TypeError [ERR_INVALID_URL]: Invalid URL at __node_internal_captureLargerStackTrace (<node_internals>/internal/errors:484:5) at NodeError (<node_internals>/internal/errors:393:5) at onParseError (<node_internals>/internal/url:565:9) at URL (<node_internals>/internal/url:645:5) at onRequestCreated (/Users/Documents/node_modules/@opentelemetry/instrumentation-undici/build/src/undici.js:108:28) at publish (<node_internals>/diagnostics_channel:56:9) at Request (/Users/Documents/node_modules/undici/lib/core/request.js:141:23) at dispatch (/Users/Documents/node_modules/undici/lib/client.js:294:23) at request (/Users/Documents/node_modules/undici/lib/api/api-request.js:154:10) at <anonymous> (/Users/Documents/node_modules/undici/lib/api/api-request.js:147:15) at request (/Users/Documents/node_modules/undici/lib/api/api-request.js:146:12) at doRequest (/Users/Documents/node_modules/solr-client/dist/lib/solr.js:154:50) at doQuery (/Users/Documents/node_modules/solr-client/dist/lib/solr.js:435:21) at search (/Users/Documents/node_modules/solr-client/dist/lib/solr.js:370:21) at getQuery (/Users/Documents/src/helpers/solr-query.js:77:37)

This error is only because of this feature is currently not supported for older version of undici.

If yes, this help me where should I have to do changes. otherwise explain more on this.

Thanks.

@pichlermarc
Copy link
Member

pichlermarc commented Oct 21, 2024

@neo1parekh ah, I see - looks like something inside the instrumentation is throwing here.

I'm not sure though if this is related

  • to the version of undici that's used OR
  • the specific input that's provided from solr-client to the undici request function.

Looking at the error above - would you mind providing some additional info that's logged to the console (other than the stack trace)? This is what an error thrown by the URL constructor usually looks like:

Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:496:5)
    at new NodeError (node:internal/errors:405:5)
    at new URL (node:internal/url:676:13) {
  input: 'foo',
  code: 'ERR_INVALID_URL'
}

it's the input: and code: are what would be most helpful in tracking this down.

@pichlermarc pichlermarc added bug Something isn't working triage needs:reproducer This bug/feature is in need of a minimal reproducer and removed discussion labels Oct 23, 2024
@pichlermarc
Copy link
Member

I'm re-categorizing this as a possible bug that is in need for a reproducer - I've tried to reproduce this by passing various inputs to [email protected] but nothing I could come up with actually triggered the error for me 😞

@trentm trentm changed the title Help needed with package conflict. instrumentation-undici can throw "[ERR_INVALID_URL]: Invalid URL" in 'undici:request:create' handler Nov 5, 2024
@trentm
Copy link
Contributor

trentm commented Nov 5, 2024

repro

Here is my repro:

use-solr.js

const solr = require('solr-client');
async function main() {
    const client = solr.createClient(
        { host: '127.0.0.1', port: 8983, path: '/solr', core: 'gettingstarted' }
    );
    const res = await client.ping();
    console.log('solr response: ', res);
}
main()

Using that:

# start solr
mkdir -p solrdata
docker run -d -v "$PWD/solrdata:/var/solr" -p 8983:8983 --name my_solr solr:8 solr-precreate gettingstarted

node use-solr.js

That works without instrumentation:

% node use-solr.js
solr response:  {
  responseHeader: {
    zkConnected: null,
    status: 0,
    QTime: 2,
    params: {
      q: '{!lucene}*:*',
      distrib: 'false',
      df: '_text_',
      rows: '10',
      wt: 'json',
      echoParams: 'all',
      rid: '-3'
    }
  },
  status: 'OK'
}

Now add instrumentation:

% npm ls
[email protected] /Users/trentm/tmp/asdf.20241105T113812
├── @opentelemetry/[email protected]
└── [email protected]

% 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
node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

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/tmp/asdf.20241105T113812/node_modules/@opentelemetry/instrumentation-undici/build/src/undici.js:125:28)
    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'
}

Node.js v18.20.4

the issue in instr-undici

instr-undici is failing on:

  private onRequestCreated({ request }: RequestMessage): void {
    // ...
    const requestUrl = new URL(request.origin + request.path);

In the above example, courtesy of debugging prints, the request object has:

XXX request:  Request {
  headersTimeout: undefined,
  bodyTimeout: undefined,
  method: 'GET',
...
  path: 'http://127.0.0.1:8983/solr/gettingstarted/admin/ping?wt=json',
  origin: 'http://127.0.0.1:8983',

That path value being a full URL is surprising -- at least to undici instrumentation.
A more typical example would have:

XXX request:  Request {
  headersTimeout: undefined,
  bodyTimeout: undefined,
  method: 'GET',
...
  path: '/solr/gettingstarted/admin/ping?wt=json',
  origin: 'http://127.0.0.1:8983',

However, instr-undici needs to be defensive.

an issue in solr-client?

The solr-client passing a path value being a full URL is odd. It results in the solr server receiving an HTTP request like this:

    GET http://127.0.0.1:8983/solr/gettingstarted/admin/ping?wt=json HTTP/1.1
    host: 127.0.0.1:8983
    connection: keep-alive
    accept: application/json; charset=utf-8

I'm not sure if this is for some sort of proxying by Solr to multiple nodes or just a mis-usage by the Node.js solr-client.
I hacked solr-client to put bogus values in the host and port parts of the path, e.g.: undiciClient.request(path: "http://127.0.0.1bogus:8984/solr/gettingstarted/admin/ping?wt=json", ...) and the Solr server was fine with it. I think the Solr server is (accidentally or intentionally) throwing away all but the path part of that URL.

@trentm
Copy link
Contributor

trentm commented Nov 5, 2024

fix options

First, the new URL(...) usage in instr-undici should be guarded by a try/catch and abort instrumenting the request if it gets a surprise.


However, should it do better than the current:

const requestUrl = new URL(request.origin + request.path);

?

If request.origin is http://example1.com and request.path is http://example2.com/some/path what should OTel capture as the full URL of this client request? Is it http://example2.com/some/path?

We could change the code to:

const requestUrl = new URL(request.path, request.origin);

Then we get these cases:

// The usual case:
> new URL('/some/path', 'http://example1.com').href
'http://example1.com/some/path'

// The case on this issue where request.path is a full URL
> new URL('http://example2.com:8001/some/path', 'http://example1.com:8000').href
'http://example2.com:8001/some/path'

I think this could work with the HTTP client span semantics (https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client). Ideally we would get these span attributes:

url.full: 'http://example2.com:8001/some/path'
server.address: example1.com
server.port: 8000

@trentm trentm removed triage needs:reproducer This bug/feature is in need of a minimal reproducer labels Nov 5, 2024
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Nov 5, 2024
@trentm trentm closed this as completed in 28e209a Nov 7, 2024
@neo1parekh
Copy link
Author

Hi Folks,

Thank you so much for looking into it on, and provided best solution on my small query.

Really appreciated for time you all have put in 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:instrumentation-undici
Projects
None yet
3 participants