feat(core): Export a reusable function to add tracing headers#20076
Open
feat(core): Export a reusable function to add tracing headers#20076
Conversation
Contributor
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛Core
Other
Internal Changes 🔧Core
Deps
Other
🤖 This preview updates automatically when you update the PR. |
Contributor
size-limit report 📦
|
Contributor
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| type PolymorphicRequestHeaders = | ||
| | Record<string, string | undefined> | ||
| | Array<[string, string]> | ||
| | Iterable<Iterable<string>> |
There was a problem hiding this comment.
Accepted Iterable type lacks runtime handling in function
Low Severity
The new Iterable<Iterable<string>> variant was added to PolymorphicRequestHeaders, which is used as the type for fetchOptionsObj.headers. However, the function body only handles Headers instances (via isHeaders), arrays (via Array.isArray), and plain objects (the else branch). A non-array, non-Headers iterable (e.g. a Map) would silently fall into the else branch, where in operator checks and object spread would not correctly process the iterable's entries, producing malformed headers. Since this function is now exported for cross-package use, the type promises broader acceptance than the implementation supports.
Additional Locations (1)
Member
Author
There was a problem hiding this comment.
This type is needed for Cloudflare headers. I've added a test to also check for that case.
JPeer264
force-pushed
the
jp/export-trace-headers
branch
from
d9322e7 to
1622527
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: feat PR lacks integration or E2E test
- Added a Node.js integration test that demonstrates cross-package usage of getTracingHeadersForFetchRequest, validating that the function correctly generates tracing headers when imported from @sentry/core.
Or push these changes by commenting:
@cursor push 7c341385d0
Preview (7c341385d0)
diff --git a/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/scenario.ts
new file mode 100644
--- /dev/null
+++ b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/scenario.ts
@@ -1,0 +1,30 @@
+import * as Sentry from '@sentry/node';
+import { getTracingHeadersForFetchRequest } from '@sentry/core';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+ dsn: 'https://public@dsn.ingest.sentry.io/1337',
+ tracesSampleRate: 1.0,
+ transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'test-span' }, () => {
+ const headers = getTracingHeadersForFetchRequest('https://example.com/api', {});
+
+ if (!headers || typeof headers !== 'object' || Array.isArray(headers)) {
+ throw new Error('getTracingHeadersForFetchRequest returned invalid headers');
+ }
+
+ const sentryTrace = 'sentry-trace' in headers ? headers['sentry-trace'] : undefined;
+ const baggage = 'baggage' in headers ? headers.baggage : undefined;
+
+ if (!sentryTrace || typeof sentryTrace !== 'string' || !sentryTrace.match(/^[0-9a-f]{32}-[0-9a-f]{16}-[01]$/)) {
+ throw new Error('sentry-trace header is invalid or missing');
+ }
+
+ if (!baggage || typeof baggage !== 'string' || !baggage.includes('sentry-')) {
+ throw new Error('baggage header is invalid or missing');
+ }
+
+ Sentry.captureMessage('test message');
+});
diff --git a/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/test.ts b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/test.ts
new file mode 100644
--- /dev/null
+++ b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/test.ts
@@ -1,0 +1,17 @@
+import { afterAll, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+ cleanupChildProcesses();
+});
+
+test('should generate tracing headers for fetch request', async () => {
+ await createRunner(__dirname, 'scenario.ts')
+ .expect({
+ event: {
+ message: 'test message',
+ },
+ })
+ .start()
+ .completed();
+});This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
JPeer264
requested review from
chargome and
s1gr1d
and removed request for
Lms24 and
nicohrubec
Member
|
q: IIC you want to use this in Cloudflare afterwards? I think we have serveral conventions for internal functions but lately we've been making use of an |
Member
Author
|
Yes for now only for Cloudflare, but in theory this can be then used for other instrumentations then. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8100e43. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
JPeer264
force-pushed
the
jp/export-trace-headers
branch
from
8100e43 to
4e235e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is an extraction of #19991
It basically exports
getTracingHeadersForFetchRequest, which was previously only exported for testing, but offers a great functionality if you want to add tracing headers to a request. I renamed it asaddTracingHeadersToFetchRequestsounded a little misleading, as it didn't really add headers to the request, as it returned the extracted headers from the request (or init, if there are any).Open question
I added
@hiddenand@internalto it, not sure if this is an approach we follow. I'm ok to remove it from the jsdoc