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

global fetch does not propagate the tracing headers #3443

Closed
Grmiade opened this issue Jul 21, 2023 · 2 comments · Fixed by #3449
Closed

global fetch does not propagate the tracing headers #3443

Grmiade opened this issue Jul 21, 2023 · 2 comments · Fixed by #3449
Labels
bug Something isn't working

Comments

@Grmiade
Copy link

Grmiade commented Jul 21, 2023

The global fetch support has been introduced in this PR #3258.
But the injected headers in order to link the traces between them are not correctly configured.

Expected behaviour
When I use global fetch the tracing headers should be injected in order to link related traces.

Actual behaviour
Currently, the tracing headers are not correctly injected.

Detailed description
https://github.com/DataDog/dd-trace-js/blob/master/packages/datadog-instrumentations/src/fetch.js#L24
We are using message.headers but this object does not contain the injected headers.
Something is probably missing here: https://github.com/DataDog/dd-trace-js/blob/master/packages/datadog-plugin-fetch/src/index.js#L22C5-L22C59
We should probably make message.headers = args.options.headers in order to take advantage of https://github.com/DataDog/dd-trace-js/blob/master/packages/datadog-plugin-http/src/client.js#L66.
BTW Are we sure the _inject method is useful?

Environment

  • Node.js version: 20.4.0
  • Tracer version: 4.7.0
@Grmiade Grmiade added the bug Something isn't working label Jul 21, 2023
@rochdev
Copy link
Member

rochdev commented Jul 21, 2023

Do you have a reproduction snippet? In our tests propagation seems to be working properly but we might be missing some case.

BTW Are we sure the _inject method is useful?

It's not and I removed it in #3438.

@Grmiade
Copy link
Author

Grmiade commented Jul 22, 2023

Hi @rochdev 👋

I checked your test suite. And it seems I found the missing test case:

it('should inject its parent span in the existing headers', done => {
  const app = express()

  app.get('/user', (req, res) => {
    expect(req.get('foo')).to.be.a('string')
    expect(req.get('x-datadog-trace-id')).to.be.a('string')
    expect(req.get('x-datadog-parent-id')).to.be.a('string')

    res.status(200).send()
  })

  getPort().then(port => {
    agent
      .use(traces => {
        expect(traces[0][0].meta).to.have.property('http.status_code', '200')
      })
    .then(done)
    .catch(done)

    appListener = server(app, port, () => {
      fetch(`http://localhost:${port}/user?foo=bar`, { headers: { 'foo': 'bar' } })
    })
  })
})

The tracing headers are not correctly injected when we already have initial headers in the fetch request.
As explained in my description, the injected headers are not included in message.headers. And since we use it as second argument, I suppose it overrides the injected headers included in the first argument (via message.req).
See https://github.com/DataDog/dd-trace-js/blob/master/packages/datadog-instrumentations/src/fetch.js#L23

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

Successfully merging a pull request may close this issue.

2 participants