feature: add CapacitorHttp support - Issue#820 - #1387
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Thank you for your contribution! could you post some screenshots of CapacitorHttp being captured on sentry.io? |
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
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 5d4f794. Configure here.
|
|
||
| function getMethod(method: HttpMethod, options: HttpOptions): string { | ||
| return method === 'request' | ||
| ? (options.method ?? 'GET').toUpperCase() |
There was a problem hiding this comment.
GET is always uppercase so we only need to apply it only to the method if method exists
| ? (options.method ?? 'GET').toUpperCase() | |
| ? (options.method?.toUpperCase() ?? 'GET') |
|
|
||
| setupOnce(): void { | ||
| if (!Capacitor.isNativePlatform()) { | ||
| return; |
There was a problem hiding this comment.
Lets add a warning so users know why this is disabled:
| return; | |
| debug.warn(`[${INTEGRATION_NAME}]` is disabled by not running on a native platform`); | |
| return; |
Must import debug from @sentry/core
| function addTracingHeaders(options: HttpOptions, span: Span): HttpOptions { | ||
| const client = getClient(); | ||
|
|
||
| if (!client) { |
There was a problem hiding this comment.
client already exists here since the previous function that called addTracingHeaders already checked it before invoking it. I would suggest passing the client as a parameter into this function
| integrations.push(nativeReleaseIntegration()); | ||
| integrations.push(eventOriginIntegration()); | ||
| integrations.push(sdkInfoIntegration()); | ||
| integrations.push(capacitorHttpIntegration()); |
There was a problem hiding this comment.
Let's remove this line so it makes the integration optional for the time being.
| integrations.push(capacitorHttpIntegration()); |
|
Thank you for your contribution! I have a specific concern. |
| { | ||
| name: spanName, | ||
| op: 'http.client', | ||
| onlyIfParent: true, |
There was a problem hiding this comment.
Let's follow what the Sentry JavaScript is doing for deciding about the parent with
onlyIfParent: !hasSpanStreamingEnabled(client),must import hasSpanStreamingEnabled from @sentry/core
| function mergeBaggageHeader( | ||
| headers: Record<string, string>, | ||
| sentryBaggage: string | undefined, | ||
| ): void { | ||
| if (!sentryBaggage) { | ||
| return; | ||
| } | ||
|
|
||
| const existingKey = findHeaderKey(headers, 'baggage'); | ||
|
|
||
| if (!existingKey) { | ||
| headers.baggage = sentryBaggage; | ||
| return; | ||
| } | ||
|
|
||
| const existingValue = headers[existingKey]; | ||
|
|
||
| if (!existingValue) { | ||
| headers[existingKey] = sentryBaggage; | ||
| return; | ||
| } | ||
|
|
||
| // Preserve baggage which already contains Sentry Values | ||
| if (/(?:^|,)\s*sentry-[^=]*=/.test(existingValue)) { | ||
| return; | ||
| } | ||
|
|
||
| headers[existingKey] = `${existingValue},${sentryBaggage}`; | ||
| } |
There was a problem hiding this comment.
We can simplify this code reducing the if complexity
| function mergeBaggageHeader( | |
| headers: Record<string, string>, | |
| sentryBaggage: string | undefined, | |
| ): void { | |
| if (!sentryBaggage) { | |
| return; | |
| } | |
| const existingKey = findHeaderKey(headers, 'baggage'); | |
| if (!existingKey) { | |
| headers.baggage = sentryBaggage; | |
| return; | |
| } | |
| const existingValue = headers[existingKey]; | |
| if (!existingValue) { | |
| headers[existingKey] = sentryBaggage; | |
| return; | |
| } | |
| // Preserve baggage which already contains Sentry Values | |
| if (/(?:^|,)\s*sentry-[^=]*=/.test(existingValue)) { | |
| return; | |
| } | |
| headers[existingKey] = `${existingValue},${sentryBaggage}`; | |
| } | |
| function mergeBaggageHeader( | |
| headers: Record<string, string>, | |
| sentryBaggage: string | undefined, | |
| ): void { | |
| if (!sentryBaggage) { | |
| return; | |
| } | |
| const key = findHeaderKey(headers, 'baggage') ?? 'baggage'; | |
| const existingValue = headers[key]; | |
| // Preserve baggage which already contains Sentry values | |
| if (existingValue && /(?:^|,)\s*sentry-[^=]*=/.test(existingValue)) { | |
| return; | |
| } | |
| headers[key] = existingValue ? `${existingValue},${sentryBaggage}` : sentryBaggage; | |
| } |
|
Thank you for your contribution! |
2 participants
📢 Type of change
📜 Description
💡 Motivation and Context
Fixes Issue#820
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled