fix(deno): Enable sessions for HTTP requests - #23313
Conversation
|
Hey @AG0708 thanks for your contribution. May I ask if there is an existing issue for this? But I see that the JSDoc actually says that the default is |
9eb32ec to
71f66dc
Compare
|
I rebased this branch onto current |
Deno disabled release-health sessions for incoming node:http requests even though the shared HTTP instrumentation defaults them on. Preserve the shared default and cover it with a real request regression test. Co-Authored-By: OpenAI Codex <codex@openai.com> Signed-off-by: Abhinav Gorrepati <gorrepatiabhinav1@gmail.com>
3683d10 to
0e80294
Compare
|
This is the simple targeted fix that makes Pushed a |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ded3cdd. Configure here.
|
@RulaKhaled Since I see you're working on fixing up and organizing the HTTP instrumentation options, could you take a look at this? I've modified this PR a bit too much to be trusted to do the review, I'm afraid 😅 It might be a good idea to rebase onto #23443 or #23396, what do you think? |
Happy to review this! tbh I wouldn’t rebase this onto #23443 / #23396 🤔 they are Node httpIntegration cleanup and don’t touch Deno, so there’s no overlap or conflict. I recommend we land it on develop as is, the Deno option names (onIncomingSpanCreated vs onSpanCreated) can stay for now and we can rename them in a small follow up later as it's a breaking API change that doesn’t need to block this PR (i can create a ticket for this), is this ok? |
|
Are there any plans to backport this to v10? Asking because it changes how much I'd want this PR to do, though Deno has diverged a fair bit since #22790 :sadge: so it'd be a manual port either way. Two suggestions:
Either works, but now What do you think? |
|
I'd suggest that we not backport to v10, and instead go all the way with making Deno match Node, Bun, and the rest, as much as possible.
Maybe the thing to do is bite the bullet and remove the deno-specific HTTP instrumentation options entirely, and just use the same options as everywhere else, so we just need to provide the In either event, I think the thing to do is to clean up the issue you raise here about the |

Deno's
node:httpserver integration disabled release-health sessions eventhough the shared HTTP instrumentation enables them by default. Keeping the
shared default aligns Deno with the other server runtimes and restores session
aggregates for incoming requests when a release is configured.
Root cause
The Deno integration passed
sessions: falsewhen it subscribed to shared HTTPserver instrumentation. There is no Deno-specific duplicate instrumentation
that requires that override.
yarn lint) & (yarn test).Fixes #22888