From cd2bb056eeec8cd87aeee9357dbafaec363efc6a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 11:28:25 +0200 Subject: [PATCH 01/11] feat(feedback): Add `captureFeedback` method fix PR --- packages/browser/src/client.ts | 2 + packages/browser/src/exports.ts | 1 + packages/browser/src/index.bundle.feedback.ts | 2 + .../index.bundle.tracing.replay.feedback.ts | 1 + packages/browser/src/index.ts | 1 + packages/browser/src/sdk.ts | 3 + packages/core/src/feedback.ts | 71 ++++++++++++++ packages/core/src/index.ts | 1 + .../feedback/src/core/sendFeedback.test.ts | 14 ++- packages/feedback/src/core/sendFeedback.ts | 92 +++++-------------- .../feedback/src/util/prepareFeedbackEvent.ts | 43 --------- packages/types/src/feedback/sendFeedback.ts | 7 +- 12 files changed, 118 insertions(+), 120 deletions(-) create mode 100644 packages/core/src/feedback.ts delete mode 100644 packages/feedback/src/util/prepareFeedbackEvent.ts diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 78ab902e01a1..c301df98f7f6 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -91,6 +91,8 @@ export class BrowserClient extends BaseClient { /** * Sends user feedback to Sentry. + * + * @deprecated Use `captureFeedback` instead. */ public captureUserFeedback(feedback: UserFeedback): void { if (!this._isEnabled()) { diff --git a/packages/browser/src/exports.ts b/packages/browser/src/exports.ts index 58391f1432c0..3da765aaa0f7 100644 --- a/packages/browser/src/exports.ts +++ b/packages/browser/src/exports.ts @@ -88,6 +88,7 @@ export { init, onLoad, showReportDialog, + // eslint-disable-next-line deprecation/deprecation captureUserFeedback, } from './sdk'; diff --git a/packages/browser/src/index.bundle.feedback.ts b/packages/browser/src/index.bundle.feedback.ts index eecad6304ebf..957583d79eeb 100644 --- a/packages/browser/src/index.bundle.feedback.ts +++ b/packages/browser/src/index.bundle.feedback.ts @@ -11,3 +11,5 @@ export { feedbackAsyncIntegration as feedbackIntegration, replayIntegrationShim as replayIntegration, }; + +export { captureFeedback } from '@sentry/core'; diff --git a/packages/browser/src/index.bundle.tracing.replay.feedback.ts b/packages/browser/src/index.bundle.tracing.replay.feedback.ts index 194a387bf4c4..de8453db8784 100644 --- a/packages/browser/src/index.bundle.tracing.replay.feedback.ts +++ b/packages/browser/src/index.bundle.tracing.replay.feedback.ts @@ -13,6 +13,7 @@ export { withActiveSpan, getSpanDescendants, setMeasurement, + captureFeedback, } from '@sentry/core'; export { diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 04a98fbacb02..86e6ea20fe81 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -10,6 +10,7 @@ export { extraErrorDataIntegration, rewriteFramesIntegration, sessionTimingIntegration, + captureFeedback, } from '@sentry/core'; export { diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index d64981cb0c7a..422acf8c3150 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -295,10 +295,13 @@ function startSessionTracking(): void { /** * Captures user feedback and sends it to Sentry. + * + * @deprecated Use `captureFeedback` instead. */ export function captureUserFeedback(feedback: UserFeedback): void { const client = getClient(); if (client) { + // eslint-disable-next-line deprecation/deprecation client.captureUserFeedback(feedback); } } diff --git a/packages/core/src/feedback.ts b/packages/core/src/feedback.ts new file mode 100644 index 000000000000..f1acda999d44 --- /dev/null +++ b/packages/core/src/feedback.ts @@ -0,0 +1,71 @@ +import type { Attachment, EventHint, FeedbackEvent } from '@sentry/types'; +import { getClient, getCurrentScope } from './currentScopes'; +import { createAttachmentEnvelope } from './envelope'; + +interface FeedbackParams { + message: string; + name?: string; + email?: string; + attachments?: Attachment[]; + url?: string; + source?: string; + associatedEventId?: string; +} + +/** + * Send user feedback to Sentry. + */ +export function captureFeedback( + feedbackParams: FeedbackParams, + hint?: EventHint & { includeReplay?: boolean }, +): string { + const { message, name, email, url, source, attachments, associatedEventId } = feedbackParams; + + const client = getClient(); + const transport = client && client.getTransport(); + const dsn = client && client.getDsn(); + + if (!client || !transport || !dsn) { + throw new Error('Invalid Sentry client'); + } + + const feedbackEvent: FeedbackEvent = { + contexts: { + feedback: { + contact_email: email, + name, + message, + url, + source, + associated_event_id: associatedEventId, + }, + }, + type: 'feedback', + level: 'info', + }; + + if (client) { + client.emit('beforeSendFeedback', feedbackEvent, hint); + } + + const eventId = getCurrentScope().captureEvent(feedbackEvent, hint); + + // For now, we have to send attachments manually in a separate envelope + // Because we do not support attachments in the feedback envelope + // Once the Sentry API properly supports this, we can get rid of this and send it through the event envelope + if (client && attachments && attachments.length) { + // TODO: https://docs.sentry.io/platforms/javascript/enriching-events/attachments/ + // eslint-disable-next-line @typescript-eslint/no-floating-promises + void transport.send( + createAttachmentEnvelope( + feedbackEvent, + attachments, + dsn, + client.getOptions()._metadata, + client.getOptions().tunnel, + ), + ); + } + + return eventId; +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c2c210262483..c781d4f10d46 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -99,6 +99,7 @@ export { BrowserMetricsAggregator } from './metrics/browser-aggregator'; export { getMetricSummaryJsonForSpan } from './metrics/metric-summary'; export { addTracingHeadersToFetchRequest, instrumentFetchRequest } from './fetch'; export { trpcMiddleware } from './trpc'; +export { captureFeedback } from './feedback'; // eslint-disable-next-line deprecation/deprecation export { getCurrentHubShim, getCurrentHub } from './getCurrentHubShim'; diff --git a/packages/feedback/src/core/sendFeedback.test.ts b/packages/feedback/src/core/sendFeedback.test.ts index 1b0ad480ca4e..614cbdb542a9 100644 --- a/packages/feedback/src/core/sendFeedback.test.ts +++ b/packages/feedback/src/core/sendFeedback.test.ts @@ -14,18 +14,26 @@ describe('sendFeedback', () => { message: 'mi', }); expect(mockTransport).toHaveBeenCalledWith([ - { event_id: expect.any(String), sent_at: expect.any(String) }, + { + event_id: expect.any(String), + sent_at: expect.any(String), + trace: expect.anything(), + }, [ [ { type: 'feedback' }, { breadcrumbs: undefined, contexts: { + trace: { + parent_span_id: undefined, + span_id: expect.any(String), + trace_id: expect.any(String), + }, feedback: { contact_email: 're@example.org', message: 'mi', name: 'doe', - replay_id: undefined, source: 'api', url: 'http://localhost/', }, @@ -33,7 +41,7 @@ describe('sendFeedback', () => { level: 'info', environment: 'production', event_id: expect.any(String), - platform: 'javascript', + // TODO: Why is there no platform here? timestamp: expect.any(Number), type: 'feedback', }, diff --git a/packages/feedback/src/core/sendFeedback.ts b/packages/feedback/src/core/sendFeedback.ts index 5aa795fb16ca..7a0abe145b2a 100644 --- a/packages/feedback/src/core/sendFeedback.ts +++ b/packages/feedback/src/core/sendFeedback.ts @@ -1,8 +1,9 @@ -import { createAttachmentEnvelope, createEventEnvelope, getClient, withScope } from '@sentry/core'; -import type { FeedbackEvent, SendFeedback, SendFeedbackParams } from '@sentry/types'; +import { captureFeedback } from '@sentry/core'; +import { getClient } from '@sentry/core'; +import type { SendFeedback, SendFeedbackParams, TransportMakeRequestResponse } from '@sentry/types'; +import type { Event } from '@sentry/types'; import { getLocationHref } from '@sentry/utils'; -import { FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE } from '../constants'; -import { prepareFeedbackEvent } from '../util/prepareFeedbackEvent'; +import { FEEDBACK_API_SOURCE } from '../constants'; /** * Public API to send a Feedback item to Sentry @@ -10,91 +11,44 @@ import { prepareFeedbackEvent } from '../util/prepareFeedbackEvent'; export const sendFeedback: SendFeedback = ( { name, email, message, attachments, source = FEEDBACK_API_SOURCE, url = getLocationHref() }: SendFeedbackParams, { includeReplay = true } = {}, -) => { +): Promise => { if (!message) { throw new Error('Unable to submit feedback with empty message'); } + // We want to wait for the feedback to be sent (or not) const client = getClient(); - const transport = client && client.getTransport(); - const dsn = client && client.getDsn(); - if (!client || !transport || !dsn) { - throw new Error('Invalid Sentry client'); + if (!client) { + throw new Error('No client setup, cannot send feedback.'); } - const baseEvent: FeedbackEvent = { - contexts: { - feedback: { - contact_email: email, - name, - message, - url, - source, - }, - }, - type: 'feedback', - }; + const eventId = captureFeedback({ name, email, message, attachments, source, url }, { includeReplay }); - return withScope(async scope => { - // No use for breadcrumbs in feedback - scope.clearBreadcrumbs(); + // We want to wait for the feedback to be sent (or not) + return new Promise((resolve, reject) => { + // After 5s, we want to clear anyhow + const timeout = setTimeout(() => reject('timeout'), 5_000); - if ([FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE].includes(String(source))) { - scope.setLevel('info'); - } - - const feedbackEvent = await prepareFeedbackEvent({ - scope, - client, - event: baseEvent, - }); - - if (client.emit) { - client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) }); - } - - try { - const response = await transport.send( - createEventEnvelope(feedbackEvent, dsn, client.getOptions()._metadata, client.getOptions().tunnel), - ); - - if (attachments && attachments.length) { - // TODO: https://docs.sentry.io/platforms/javascript/enriching-events/attachments/ - await transport.send( - createAttachmentEnvelope( - feedbackEvent, - attachments, - dsn, - client.getOptions()._metadata, - client.getOptions().tunnel, - ), - ); + client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => { + if (event.event_id !== eventId) { + return; } + clearTimeout(timeout); + // Require valid status codes, otherwise can assume feedback was not sent successfully if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) { if (response.statusCode === 0) { - throw new Error( + return reject( 'Unable to send Feedback. This is because of network issues, or because you are using an ad-blocker.', ); } - throw new Error('Unable to send Feedback. Invalid response from server.'); + return reject('Unable to send Feedback. Invalid response from server.'); } - return response; - } catch (err) { - const error = new Error('Unable to send Feedback'); - - try { - // In case browsers don't allow this property to be writable - // @ts-expect-error This needs lib es2022 and newer - error.cause = err; - } catch { - // nothing to do - } - throw error; - } + resolve(); + }); }); }; diff --git a/packages/feedback/src/util/prepareFeedbackEvent.ts b/packages/feedback/src/util/prepareFeedbackEvent.ts deleted file mode 100644 index 0d66d251c6ff..000000000000 --- a/packages/feedback/src/util/prepareFeedbackEvent.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { getIsolationScope, prepareEvent } from '@sentry/core'; -import type { Client, FeedbackEvent, Scope } from '@sentry/types'; - -interface PrepareFeedbackEventParams { - client: Client; - event: FeedbackEvent; - scope: Scope; -} -/** - * Prepare a feedback event & enrich it with the SDK metadata. - */ -export async function prepareFeedbackEvent({ - client, - scope, - event, -}: PrepareFeedbackEventParams): Promise { - const eventHint = {}; - if (client.emit) { - client.emit('preprocessEvent', event, eventHint); - } - - const preparedEvent = (await prepareEvent( - client.getOptions(), - event, - eventHint, - scope, - client, - getIsolationScope(), - )) as FeedbackEvent | null; - - if (preparedEvent === null) { - // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions - client.recordDroppedEvent('event_processor', 'feedback', event); - throw new Error('Unable to prepare event'); - } - - // This normally happens in browser client "_prepareEvent" - // but since we do not use this private method from the client, but rather the plain import - // we need to do this manually. - preparedEvent.platform = preparedEvent.platform || 'javascript'; - - return preparedEvent; -} diff --git a/packages/types/src/feedback/sendFeedback.ts b/packages/types/src/feedback/sendFeedback.ts index 612e0cff001d..7ba6600662bd 100644 --- a/packages/types/src/feedback/sendFeedback.ts +++ b/packages/types/src/feedback/sendFeedback.ts @@ -1,6 +1,5 @@ import type { Attachment } from '../attachment'; import type { Event } from '../event'; -import type { TransportMakeRequestResponse } from '../transport'; import type { User } from '../user'; /** @@ -19,6 +18,7 @@ interface FeedbackContext extends Record { name?: string; replay_id?: string; url?: string; + associated_event_id?: string; } /** @@ -48,7 +48,4 @@ interface SendFeedbackOptions { includeReplay?: boolean; } -export type SendFeedback = ( - params: SendFeedbackParams, - options?: SendFeedbackOptions, -) => Promise; +export type SendFeedback = (params: SendFeedbackParams, options?: SendFeedbackOptions) => Promise; From 7f0aba5249808a2377f21b4f84b586240bbe4464 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 11:31:30 +0200 Subject: [PATCH 02/11] test(browser-integration-test): Add tests to check trace lifetime for user feedback events --- .../suites/tracing/trace-lifetime/init.js | 2 +- .../tracing/trace-lifetime/navigation/test.ts | 48 +++++++++++++++++-- .../trace-lifetime/pageload-meta/test.ts | 37 ++++++++++++++ .../tracing/trace-lifetime/pageload/test.ts | 36 ++++++++++++++ 4 files changed, 119 insertions(+), 4 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js index 7cd076a052e5..9c34f4d99f69 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js @@ -4,7 +4,7 @@ window.Sentry = Sentry; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', - integrations: [Sentry.browserTracingIntegration()], + integrations: [Sentry.browserTracingIntegration(), Sentry.feedbackIntegration()], tracePropagationTargets: ['http://example.com'], tracesSampleRate: 1, }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index c3e4bf936288..db7c49cbe1c9 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -134,7 +134,7 @@ sentryTest('error during navigation has new navigation traceId', async ({ getLoc const url = await getLocalTestUrl({ testDir: __dirname }); - // ensure navigation transaction is finished + // ensure pageload transaction is finished await getFirstSentryEnvelopeRequest(page, url); const envelopeRequestsPromise = getMultipleSentryEnvelopeRequests( @@ -202,7 +202,7 @@ sentryTest( }); }); - // ensure navigation transaction is finished + // ensure pageload transaction is finished await getFirstSentryEnvelopeRequest(page, url); const [navigationEvent, navigationTraceHeader] = await getFirstSentryEnvelopeRequest( @@ -276,7 +276,7 @@ sentryTest( }); }); - // ensure navigation transaction is finished + // ensure pageload transaction is finished await getFirstSentryEnvelopeRequest(page, url); const navigationEventPromise = getFirstSentryEnvelopeRequest( @@ -456,3 +456,45 @@ sentryTest( ); }, ); + +sentryTest( + 'user feedback event after navigation has navigation traceId in headers', + async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + // ensure pageload transaction is finished + await getFirstSentryEnvelopeRequest(page, url); + + const navigationEvent = await getFirstSentryEnvelopeRequest(page, `${url}#foo`); + + const navigationTraceContext = navigationEvent.contexts?.trace; + expect(navigationTraceContext).toMatchObject({ + op: 'navigation', + trace_id: expect.stringMatching(/^[0-9a-f]{32}$/), + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + }); + expect(navigationTraceContext).not.toHaveProperty('parent_span_id'); + + const feedbackEventPromise = getFirstSentryEnvelopeRequest(page); + + await page.getByText('Report a Bug').click(); + expect(await page.locator(':visible:text-is("Report a Bug")').count()).toEqual(1); + await page.locator('[name="name"]').fill('Jane Doe'); + await page.locator('[name="email"]').fill('janedoe@example.org'); + await page.locator('[name="message"]').fill('my example feedback'); + await page.locator('[data-sentry-feedback] .btn--primary').click(); + + const feedbackEvent = await feedbackEventPromise; + const feedbackTraceContext = feedbackEvent.contexts?.trace; + + expect(feedbackTraceContext).toMatchObject({ + op: 'navigation', + trace_id: navigationTraceContext?.trace_id, + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + }); + }, +); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index f206e9292b9f..7b3407c7aa68 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -429,3 +429,40 @@ sentryTest( expect(headers['baggage']).toBe(META_TAG_BAGGAGE); }, ); + +sentryTest('user feedback event after pageload has pageload traceId in headers', async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + const pageloadEvent = await getFirstSentryEnvelopeRequest(page, url); + const pageloadTraceContext = pageloadEvent.contexts?.trace; + + expect(pageloadTraceContext).toMatchObject({ + op: 'pageload', + trace_id: META_TAG_TRACE_ID, + parent_span_id: META_TAG_PARENT_SPAN_ID, + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + }); + + const feedbackEventPromise = getFirstSentryEnvelopeRequest(page); + + await page.getByText('Report a Bug').click(); + expect(await page.locator(':visible:text-is("Report a Bug")').count()).toEqual(1); + await page.locator('[name="name"]').fill('Jane Doe'); + await page.locator('[name="email"]').fill('janedoe@example.org'); + await page.locator('[name="message"]').fill('my example feedback'); + await page.locator('[data-sentry-feedback] .btn--primary').click(); + + const feedbackEvent = await feedbackEventPromise; + const feedbackTraceContext = feedbackEvent.contexts?.trace; + + expect(feedbackTraceContext).toMatchObject({ + op: 'pageload', + trace_id: META_TAG_TRACE_ID, + parent_span_id: META_TAG_PARENT_SPAN_ID, + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + }); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 4bfe8c26d5cb..9049078ee2ba 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -431,3 +431,39 @@ sentryTest( ); }, ); + +sentryTest('user feedback event after pageload has pageload traceId in headers', async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + const pageloadEvent = await getFirstSentryEnvelopeRequest(page, url); + const pageloadTraceContext = pageloadEvent.contexts?.trace; + + expect(pageloadTraceContext).toMatchObject({ + op: 'pageload', + trace_id: expect.stringMatching(/^[0-9a-f]{32}$/), + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + }); + expect(pageloadTraceContext).not.toHaveProperty('parent_span_id'); + + const feedbackEventPromise = getFirstSentryEnvelopeRequest(page); + + await page.getByText('Report a Bug').click(); + expect(await page.locator(':visible:text-is("Report a Bug")').count()).toEqual(1); + await page.locator('[name="name"]').fill('Jane Doe'); + await page.locator('[name="email"]').fill('janedoe@example.org'); + await page.locator('[name="message"]').fill('my example feedback'); + await page.locator('[data-sentry-feedback] .btn--primary').click(); + + const feedbackEvent = await feedbackEventPromise; + const feedbackTraceContext = feedbackEvent.contexts?.trace; + + expect(feedbackTraceContext).toMatchObject({ + op: 'pageload', + trace_id: pageloadTraceContext?.trace_id, + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + }); +}); From 4e71967ec06cc0848a0d0d119c962ef314d8e1ed Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 11:43:10 +0200 Subject: [PATCH 03/11] fix tests etc. --- .../suites/feedback/captureFeedback/test.ts | 4 + .../hasSampling/test.ts | 4 + .../trace-lifetime/pageload-meta/test.ts | 2 +- .../tracing/trace-lifetime/pageload/test.ts | 2 +- packages/core/src/baseclient.ts | 5 +- packages/core/src/feedback.ts | 10 +- .../feedback/src/core/sendFeedback.test.ts | 340 +++++++++++++++++- packages/feedback/src/core/sendFeedback.ts | 14 +- packages/types/src/feedback/sendFeedback.ts | 2 +- 9 files changed, 365 insertions(+), 18 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/feedback/captureFeedback/test.ts b/dev-packages/browser-integration-tests/suites/feedback/captureFeedback/test.ts index b58efe858f25..01e56a9c1946 100644 --- a/dev-packages/browser-integration-tests/suites/feedback/captureFeedback/test.ts +++ b/dev-packages/browser-integration-tests/suites/feedback/captureFeedback/test.ts @@ -53,6 +53,10 @@ sentryTest('should capture feedback', async ({ getLocalTestPath, page }) => { source: 'widget', url: expect.stringContaining('/dist/index.html'), }, + trace: { + trace_id: expect.stringMatching(/\w{32}/), + span_id: expect.stringMatching(/\w{16}/), + }, }, level: 'info', timestamp: expect.any(Number), diff --git a/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackAndReplay/hasSampling/test.ts b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackAndReplay/hasSampling/test.ts index 6768bf838e75..3c46d1c79964 100644 --- a/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackAndReplay/hasSampling/test.ts +++ b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackAndReplay/hasSampling/test.ts @@ -89,6 +89,10 @@ sentryTest('should capture feedback', async ({ forceFlushReplay, getLocalTestPat source: 'widget', url: expect.stringContaining('/dist/index.html'), }, + trace: { + trace_id: expect.stringMatching(/\w{32}/), + span_id: expect.stringMatching(/\w{16}/), + }, }, level: 'info', timestamp: expect.any(Number), diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index 7b3407c7aa68..7dede24a8337 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { SpanEnvelope } from '@sentry/types'; +import type { SpanEnvelope, Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 9049078ee2ba..3b17a433a0a4 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { SpanEnvelope } from '@sentry/types'; +import type { SpanEnvelope, Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 455cba2d9771..68a113e5ff2c 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -35,6 +35,7 @@ import { addItemToEnvelope, checkOrSetAlreadyCaught, createAttachmentEnvelopeItem, + dropUndefinedKeys, isParameterizedString, isPlainObject, isPrimitive, @@ -663,11 +664,11 @@ export abstract class BaseClient implements Client { if (!trace && propagationContext) { const { traceId: trace_id, spanId, parentSpanId, dsc } = propagationContext; evt.contexts = { - trace: { + trace: dropUndefinedKeys({ trace_id, span_id: spanId, parent_span_id: parentSpanId, - }, + }), ...evt.contexts, }; diff --git a/packages/core/src/feedback.ts b/packages/core/src/feedback.ts index f1acda999d44..adedf9b4d1da 100644 --- a/packages/core/src/feedback.ts +++ b/packages/core/src/feedback.ts @@ -1,4 +1,5 @@ import type { Attachment, EventHint, FeedbackEvent } from '@sentry/types'; +import { dropUndefinedKeys } from '@sentry/utils'; import { getClient, getCurrentScope } from './currentScopes'; import { createAttachmentEnvelope } from './envelope'; @@ -31,14 +32,14 @@ export function captureFeedback( const feedbackEvent: FeedbackEvent = { contexts: { - feedback: { + feedback: dropUndefinedKeys({ contact_email: email, name, message, url, source, associated_event_id: associatedEventId, - }, + }), }, type: 'feedback', level: 'info', @@ -58,7 +59,10 @@ export function captureFeedback( // eslint-disable-next-line @typescript-eslint/no-floating-promises void transport.send( createAttachmentEnvelope( - feedbackEvent, + { + ...feedbackEvent, + event_id: eventId, + }, attachments, dsn, client.getOptions()._metadata, diff --git a/packages/feedback/src/core/sendFeedback.test.ts b/packages/feedback/src/core/sendFeedback.test.ts index 614cbdb542a9..2b139874783f 100644 --- a/packages/feedback/src/core/sendFeedback.test.ts +++ b/packages/feedback/src/core/sendFeedback.test.ts @@ -1,23 +1,60 @@ -import { getClient } from '@sentry/core'; +import { + addBreadcrumb, + getClient, + getCurrentScope, + getIsolationScope, + startSpan, + withIsolationScope, + withScope, +} from '@sentry/core'; import { mockSdk } from './mockSdk'; import { sendFeedback } from './sendFeedback'; +import { TextDecoder, TextEncoder } from 'util'; +const patchedEncoder = (!global.window.TextEncoder && (global.window.TextEncoder = TextEncoder)) || true; +// @ts-expect-error patch the encoder on the window, else importing JSDOM fails (deleted in afterAll) +const patchedDecoder = (!global.window.TextDecoder && (global.window.TextDecoder = TextDecoder)) || true; + describe('sendFeedback', () => { + beforeEach(() => { + getIsolationScope().clear(); + getCurrentScope().clear(); + jest.clearAllMocks(); + }); + + afterAll(() => { + // @ts-expect-error patch the encoder on the window, else importing JSDOM fails + patchedEncoder && delete global.window.TextEncoder; + // @ts-expect-error patch the encoder on the window, else importing JSDOM fails + patchedDecoder && delete global.window.TextDecoder; + }); + it('sends feedback', async () => { mockSdk(); const mockTransport = jest.spyOn(getClient()!.getTransport()!, 'send'); - await sendFeedback({ + const promise = sendFeedback({ name: 'doe', email: 're@example.org', message: 'mi', }); + + expect(promise).toBeInstanceOf(Promise); + + const eventId = await promise; + + expect(typeof eventId).toEqual('string'); + expect(mockTransport).toHaveBeenCalledWith([ { event_id: expect.any(String), sent_at: expect.any(String), - trace: expect.anything(), + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'dsn', + }, }, [ [ @@ -26,7 +63,6 @@ describe('sendFeedback', () => { breadcrumbs: undefined, contexts: { trace: { - parent_span_id: undefined, span_id: expect.any(String), trace_id: expect.any(String), }, @@ -41,7 +77,6 @@ describe('sendFeedback', () => { level: 'info', environment: 'production', event_id: expect.any(String), - // TODO: Why is there no platform here? timestamp: expect.any(Number), type: 'feedback', }, @@ -49,4 +84,299 @@ describe('sendFeedback', () => { ], ]); }); + + it('applies active span data to feedback', async () => { + mockSdk({ sentryOptions: { enableTracing: true } }); + const mockTransport = jest.spyOn(getClient()!.getTransport()!, 'send'); + + await startSpan({ name: 'test span' }, () => { + return sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }); + }); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: expect.any(String), + sent_at: expect.any(String), + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'dsn', + sample_rate: '1', + sampled: 'true', + transaction: 'test span', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.origin': 'manual', + 'sentry.sample_rate': 1, + 'sentry.source': 'custom', + }, + origin: 'manual', + }, + feedback: { + contact_email: 're@example.org', + message: 'mi', + name: 'doe', + source: 'api', + url: 'http://localhost/', + }, + }, + level: 'info', + environment: 'production', + event_id: expect.any(String), + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); + + it('applies scope data to feedback', async () => { + mockSdk({ sentryOptions: { enableTracing: true } }); + const mockTransport = jest.spyOn(getClient()!.getTransport()!, 'send'); + + await withIsolationScope(isolationScope => { + isolationScope.setTag('test-1', 'tag'); + isolationScope.setExtra('test-1', 'extra'); + + return withScope(scope => { + scope.setTag('test-2', 'tag'); + scope.setExtra('test-2', 'extra'); + + addBreadcrumb({ message: 'test breadcrumb', timestamp: 12345 }); + + return sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }); + }); + }); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: expect.any(String), + sent_at: expect.any(String), + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'dsn', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: [{ message: 'test breadcrumb', timestamp: 12345 }], + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + contact_email: 're@example.org', + message: 'mi', + name: 'doe', + source: 'api', + url: 'http://localhost/', + }, + }, + extra: { + 'test-1': 'extra', + 'test-2': 'extra', + }, + tags: { + 'test-1': 'tag', + 'test-2': 'tag', + }, + level: 'info', + environment: 'production', + event_id: expect.any(String), + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); + + it('handles 400 transport error', async () => { + mockSdk(); + jest.spyOn(getClient()!.getTransport()!, 'send').mockImplementation(() => { + return Promise.resolve({ statusCode: 400 }); + }); + + await expect( + sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }), + ).rejects.toMatch('Unable to send Feedback. Invalid response from server.'); + }); + + it('handles 0 transport error', async () => { + mockSdk(); + jest.spyOn(getClient()!.getTransport()!, 'send').mockImplementation(() => { + return Promise.resolve({ statusCode: 0 }); + }); + + await expect( + sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }), + ).rejects.toMatch( + 'Unable to send Feedback. This is because of network issues, or because you are using an ad-blocker.', + ); + }); + + it('handles 200 transport response', async () => { + mockSdk(); + jest.spyOn(getClient()!.getTransport()!, 'send').mockImplementation(() => { + return Promise.resolve({ statusCode: 200 }); + }); + + await expect( + sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }), + ).resolves.toEqual(expect.any(String)); + }); + + it('handles timeout', async () => { + jest.useFakeTimers(); + + mockSdk(); + jest.spyOn(getClient()!.getTransport()!, 'send').mockImplementation(() => { + return new Promise(resolve => setTimeout(resolve, 10_000)); + }); + + const promise = sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }); + + jest.advanceTimersByTime(5_000); + + await expect(promise).rejects.toMatch('Unable to determine if Feedback was correctly sent.'); + + jest.useRealTimers(); + }); + + it('sends attachments', async () => { + mockSdk(); + const mockTransport = jest.spyOn(getClient()!.getTransport()!, 'send'); + + const attachment1 = new Uint8Array([1, 2, 3, 4, 5]); + const attachment2 = new Uint8Array([6, 7, 8, 9]); + + const promise = sendFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + attachments: [ + { + data: attachment1, + filename: 'test-file.txt', + }, + { + data: attachment2, + filename: 'test-file2.txt', + }, + ], + }); + + expect(promise).toBeInstanceOf(Promise); + + const eventId = await promise; + + expect(typeof eventId).toEqual('string'); + expect(mockTransport).toHaveBeenCalledTimes(2); + + const [feedbackEnvelope, attachmentEnvelope] = mockTransport.mock.calls; + + // Feedback event is sent normally in one envelope + expect(feedbackEnvelope[0]).toEqual([ + { + event_id: eventId, + sent_at: expect.any(String), + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'dsn', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + contact_email: 're@example.org', + message: 'mi', + name: 'doe', + source: 'api', + url: 'http://localhost/', + }, + }, + level: 'info', + environment: 'production', + event_id: eventId, + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + + // Attachments are sent in separate envelope + expect(attachmentEnvelope[0]).toEqual([ + { + event_id: eventId, + sent_at: expect.any(String), + }, + [ + [ + { + type: 'attachment', + length: 5, + filename: 'test-file.txt', + }, + attachment1, + ], + [ + { + type: 'attachment', + length: 4, + filename: 'test-file2.txt', + }, + attachment2, + ], + ], + ]); + }); }); diff --git a/packages/feedback/src/core/sendFeedback.ts b/packages/feedback/src/core/sendFeedback.ts index 7a0abe145b2a..3d99abfb5a70 100644 --- a/packages/feedback/src/core/sendFeedback.ts +++ b/packages/feedback/src/core/sendFeedback.ts @@ -11,7 +11,7 @@ import { FEEDBACK_API_SOURCE } from '../constants'; export const sendFeedback: SendFeedback = ( { name, email, message, attachments, source = FEEDBACK_API_SOURCE, url = getLocationHref() }: SendFeedbackParams, { includeReplay = true } = {}, -): Promise => { +): Promise => { if (!message) { throw new Error('Unable to submit feedback with empty message'); } @@ -26,9 +26,9 @@ export const sendFeedback: SendFeedback = ( const eventId = captureFeedback({ name, email, message, attachments, source, url }, { includeReplay }); // We want to wait for the feedback to be sent (or not) - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { // After 5s, we want to clear anyhow - const timeout = setTimeout(() => reject('timeout'), 5_000); + const timeout = setTimeout(() => reject('Unable to determine if Feedback was correctly sent.'), 5_000); client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => { if (event.event_id !== eventId) { @@ -38,7 +38,11 @@ export const sendFeedback: SendFeedback = ( clearTimeout(timeout); // Require valid status codes, otherwise can assume feedback was not sent successfully - if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) { + if ( + response && + typeof response.statusCode === 'number' && + (response.statusCode < 200 || response.statusCode >= 300) + ) { if (response.statusCode === 0) { return reject( 'Unable to send Feedback. This is because of network issues, or because you are using an ad-blocker.', @@ -47,7 +51,7 @@ export const sendFeedback: SendFeedback = ( return reject('Unable to send Feedback. Invalid response from server.'); } - resolve(); + resolve(eventId); }); }); }; diff --git a/packages/types/src/feedback/sendFeedback.ts b/packages/types/src/feedback/sendFeedback.ts index 7ba6600662bd..1dc0d6fa4369 100644 --- a/packages/types/src/feedback/sendFeedback.ts +++ b/packages/types/src/feedback/sendFeedback.ts @@ -48,4 +48,4 @@ interface SendFeedbackOptions { includeReplay?: boolean; } -export type SendFeedback = (params: SendFeedbackParams, options?: SendFeedbackOptions) => Promise; +export type SendFeedback = (params: SendFeedbackParams, options?: SendFeedbackOptions) => Promise; From 672a029bd8663f60469043828594373964d3ad41 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 14:00:51 +0200 Subject: [PATCH 04/11] export everywhere... --- packages/astro/src/index.server.ts | 1 + packages/aws-serverless/src/index.ts | 1 + packages/bun/src/index.ts | 1 + packages/deno/src/index.ts | 1 + packages/google-cloud-serverless/src/index.ts | 1 + packages/node/src/index.ts | 1 + packages/remix/src/index.server.ts | 1 + packages/sveltekit/src/server/index.ts | 1 + packages/vercel-edge/src/index.ts | 1 + 9 files changed, 9 insertions(+) diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index 71df2903e9eb..28b75ee5145a 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -14,6 +14,7 @@ export { captureEvent, captureMessage, captureCheckIn, + captureFeedback, withMonitor, createTransport, // eslint-disable-next-line deprecation/deprecation diff --git a/packages/aws-serverless/src/index.ts b/packages/aws-serverless/src/index.ts index 28fa4229ec39..5d166fce43c8 100644 --- a/packages/aws-serverless/src/index.ts +++ b/packages/aws-serverless/src/index.ts @@ -6,6 +6,7 @@ export { captureEvent, captureMessage, captureCheckIn, + captureFeedback, startSession, captureSession, endSession, diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index b61dea4a4d77..f0ab0d24e724 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -26,6 +26,7 @@ export { captureEvent, captureMessage, captureCheckIn, + captureFeedback, startSession, captureSession, endSession, diff --git a/packages/deno/src/index.ts b/packages/deno/src/index.ts index 2e5e0cefa657..be740ac22665 100644 --- a/packages/deno/src/index.ts +++ b/packages/deno/src/index.ts @@ -26,6 +26,7 @@ export { captureException, captureEvent, captureMessage, + captureFeedback, close, createTransport, continueTrace, diff --git a/packages/google-cloud-serverless/src/index.ts b/packages/google-cloud-serverless/src/index.ts index c480589a5ef1..d6f774d633f0 100644 --- a/packages/google-cloud-serverless/src/index.ts +++ b/packages/google-cloud-serverless/src/index.ts @@ -6,6 +6,7 @@ export { captureEvent, captureMessage, captureCheckIn, + captureFeedback, startSession, captureSession, endSession, diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 60b152f346e8..c209a5d2ab8b 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -90,6 +90,7 @@ export { captureException, captureEvent, captureMessage, + captureFeedback, captureConsoleIntegration, debugIntegration, dedupeIntegration, diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index d528db5802f0..e8b25aa2936f 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -18,6 +18,7 @@ export { captureException, captureEvent, captureMessage, + captureFeedback, createTransport, // eslint-disable-next-line deprecation/deprecation getCurrentHub, diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index 07c297de608b..29f7dfd313a5 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -11,6 +11,7 @@ export { captureEvent, captureMessage, captureCheckIn, + captureFeedback, withMonitor, createTransport, getClient, diff --git a/packages/vercel-edge/src/index.ts b/packages/vercel-edge/src/index.ts index 9c5a0705426e..031787bebe22 100644 --- a/packages/vercel-edge/src/index.ts +++ b/packages/vercel-edge/src/index.ts @@ -27,6 +27,7 @@ export { captureException, captureEvent, captureMessage, + captureFeedback, close, createTransport, flush, From a43bc1b96669f343dc57f37e86c40a49207a9f36 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 14:06:52 +0200 Subject: [PATCH 05/11] fix tests --- .../suites/tracing/trace-lifetime/navigation/test.ts | 4 +++- .../suites/tracing/trace-lifetime/pageload-meta/test.ts | 3 ++- .../suites/tracing/trace-lifetime/pageload/test.ts | 4 +++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index db7c49cbe1c9..bb0e6d7255fc 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -489,10 +489,12 @@ sentryTest( await page.locator('[data-sentry-feedback] .btn--primary').click(); const feedbackEvent = await feedbackEventPromise; + + expect(feedbackEvent.type).toEqual('feedback'); + const feedbackTraceContext = feedbackEvent.contexts?.trace; expect(feedbackTraceContext).toMatchObject({ - op: 'navigation', trace_id: navigationTraceContext?.trace_id, span_id: expect.stringMatching(/^[0-9a-f]{16}$/), }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index 7dede24a8337..39a1ea92ef8e 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -459,8 +459,9 @@ sentryTest('user feedback event after pageload has pageload traceId in headers', const feedbackEvent = await feedbackEventPromise; const feedbackTraceContext = feedbackEvent.contexts?.trace; + expect(feedbackEvent.type).toEqual('feedback'); + expect(feedbackTraceContext).toMatchObject({ - op: 'pageload', trace_id: META_TAG_TRACE_ID, parent_span_id: META_TAG_PARENT_SPAN_ID, span_id: expect.stringMatching(/^[0-9a-f]{16}$/), diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 3b17a433a0a4..6c23fc643a83 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -459,10 +459,12 @@ sentryTest('user feedback event after pageload has pageload traceId in headers', await page.locator('[data-sentry-feedback] .btn--primary').click(); const feedbackEvent = await feedbackEventPromise; + + expect(feedbackEvent.type).toEqual('feedback'); + const feedbackTraceContext = feedbackEvent.contexts?.trace; expect(feedbackTraceContext).toMatchObject({ - op: 'pageload', trace_id: pageloadTraceContext?.trace_id, span_id: expect.stringMatching(/^[0-9a-f]{16}$/), }); From 8abfca8be43b65564ecbc7bf5c48f9039226b991 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 14:57:13 +0200 Subject: [PATCH 06/11] more tests --- packages/core/src/feedback.ts | 20 +- packages/core/test/lib/feedback.test.ts | 383 ++++++++++++++++++ packages/core/test/mocks/client.ts | 7 +- .../feedback/src/core/sendFeedback.test.ts | 63 ++- packages/feedback/src/core/sendFeedback.ts | 13 +- packages/types/src/feedback/sendFeedback.ts | 1 + 6 files changed, 460 insertions(+), 27 deletions(-) create mode 100644 packages/core/test/lib/feedback.test.ts diff --git a/packages/core/src/feedback.ts b/packages/core/src/feedback.ts index adedf9b4d1da..ac78efeaa319 100644 --- a/packages/core/src/feedback.ts +++ b/packages/core/src/feedback.ts @@ -1,23 +1,13 @@ -import type { Attachment, EventHint, FeedbackEvent } from '@sentry/types'; +import type { EventHint, FeedbackEvent, SendFeedbackParams } from '@sentry/types'; import { dropUndefinedKeys } from '@sentry/utils'; import { getClient, getCurrentScope } from './currentScopes'; import { createAttachmentEnvelope } from './envelope'; -interface FeedbackParams { - message: string; - name?: string; - email?: string; - attachments?: Attachment[]; - url?: string; - source?: string; - associatedEventId?: string; -} - /** * Send user feedback to Sentry. */ export function captureFeedback( - feedbackParams: FeedbackParams, + feedbackParams: SendFeedbackParams, hint?: EventHint & { includeReplay?: boolean }, ): string { const { message, name, email, url, source, attachments, associatedEventId } = feedbackParams; @@ -26,10 +16,6 @@ export function captureFeedback( const transport = client && client.getTransport(); const dsn = client && client.getDsn(); - if (!client || !transport || !dsn) { - throw new Error('Invalid Sentry client'); - } - const feedbackEvent: FeedbackEvent = { contexts: { feedback: dropUndefinedKeys({ @@ -54,7 +40,7 @@ export function captureFeedback( // For now, we have to send attachments manually in a separate envelope // Because we do not support attachments in the feedback envelope // Once the Sentry API properly supports this, we can get rid of this and send it through the event envelope - if (client && attachments && attachments.length) { + if (client && transport && dsn && attachments && attachments.length) { // TODO: https://docs.sentry.io/platforms/javascript/enriching-events/attachments/ // eslint-disable-next-line @typescript-eslint/no-floating-promises void transport.send( diff --git a/packages/core/test/lib/feedback.test.ts b/packages/core/test/lib/feedback.test.ts new file mode 100644 index 000000000000..563fc812a138 --- /dev/null +++ b/packages/core/test/lib/feedback.test.ts @@ -0,0 +1,383 @@ +import { Span } from '@sentry/types'; +import { getCurrentScope, setCurrentClient, startSpan } from '../../src'; +import { captureFeedback } from '../../src/feedback'; +import { TestClient, getDefaultTestClientOptions } from '../mocks/client'; + +describe('captureFeedback', () => { + beforeEach(() => { + getCurrentScope().setClient(undefined); + getCurrentScope().clear(); + }); + + test('it works without a client', () => { + const res = captureFeedback({ + message: 'test', + }); + + expect(typeof res).toBe('string'); + }); + + test('it works with minimal options', async () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + enableSend: true, + }), + ); + setCurrentClient(client); + client.init(); + + const mockTransport = jest.spyOn(client.getTransport()!, 'send'); + + const eventId = captureFeedback({ + message: 'test', + }); + + await client.flush(); + + expect(typeof eventId).toBe('string'); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: eventId, + sent_at: expect.any(String), + trace: { + environment: 'production', + public_key: 'dsn', + trace_id: expect.any(String), + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + message: 'test', + }, + }, + level: 'info', + environment: 'production', + event_id: eventId, + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); + + test('it works with full options', async () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + enableSend: true, + }), + ); + setCurrentClient(client); + client.init(); + + const mockTransport = jest.spyOn(client.getTransport()!, 'send'); + + const eventId = captureFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + url: 'http://example.com/', + source: 'custom-source', + associatedEventId: '1234', + }); + + await client.flush(); + + expect(typeof eventId).toBe('string'); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: eventId, + sent_at: expect.any(String), + trace: { + environment: 'production', + public_key: 'dsn', + trace_id: expect.any(String), + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + name: 'doe', + contact_email: 're@example.org', + message: 'mi', + url: 'http://example.com/', + source: 'custom-source', + associated_event_id: '1234', + }, + }, + level: 'info', + environment: 'production', + event_id: eventId, + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); + + test('it captures attachments', async () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + enableSend: true, + }), + ); + setCurrentClient(client); + client.init(); + + const mockTransport = jest.spyOn(client.getTransport()!, 'send'); + + const attachment1 = new Uint8Array([1, 2, 3, 4, 5]); + const attachment2 = new Uint8Array([6, 7, 8, 9]); + + const eventId = captureFeedback({ + message: 'test', + attachments: [ + { + data: attachment1, + filename: 'test-file.txt', + }, + { + data: attachment2, + filename: 'test-file2.txt', + }, + ], + }); + + await client.flush(); + + expect(typeof eventId).toBe('string'); + + expect(mockTransport).toHaveBeenCalledTimes(2); + + const [feedbackEnvelope, attachmentEnvelope] = mockTransport.mock.calls; + + // Feedback event is sent normally in one envelope + expect(feedbackEnvelope[0]).toEqual([ + { + event_id: eventId, + sent_at: expect.any(String), + trace: { + environment: 'production', + public_key: 'dsn', + trace_id: expect.any(String), + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + message: 'test', + }, + }, + level: 'info', + environment: 'production', + event_id: eventId, + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + + // Attachments are sent in separate envelope + expect(attachmentEnvelope[0]).toEqual([ + { + event_id: eventId, + sent_at: expect.any(String), + }, + [ + [ + { + type: 'attachment', + length: 5, + filename: 'test-file.txt', + }, + attachment1, + ], + [ + { + type: 'attachment', + length: 4, + filename: 'test-file2.txt', + }, + attachment2, + ], + ], + ]); + }); + + test('it captures DSC from scope', async () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + enableSend: true, + }), + ); + setCurrentClient(client); + client.init(); + + const mockTransport = jest.spyOn(client.getTransport()!, 'send'); + + const traceId = '4C79F60C11214EB38604F4AE0781BFB2'; + const spanId = 'FA90FDEAD5F74052'; + const dsc = { + trace_id: traceId, + span_id: spanId, + sampled: 'true', + }; + + getCurrentScope().setPropagationContext({ + traceId, + spanId, + dsc, + }); + + const eventId = captureFeedback({ + message: 'test', + }); + + await client.flush(); + + expect(typeof eventId).toBe('string'); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: eventId, + sent_at: expect.any(String), + trace: { + trace_id: traceId, + span_id: spanId, + sampled: 'true', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + trace_id: traceId, + span_id: spanId, + }, + feedback: { + message: 'test', + }, + }, + level: 'info', + environment: 'production', + event_id: eventId, + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); + + test('it captures data from active span', async () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + enableSend: true, + enableTracing: true, + // We don't care about transactions here... + beforeSendTransaction() { + return null; + }, + }), + ); + setCurrentClient(client); + client.init(); + + const mockTransport = jest.spyOn(client.getTransport()!, 'send'); + + let span: Span | undefined; + const eventId = startSpan({ name: 'test-span' }, _span => { + span = _span; + return captureFeedback({ + message: 'test', + }); + }); + + await client.flush(); + + expect(typeof eventId).toBe('string'); + expect(span).toBeDefined(); + + const { spanId, traceId } = span!.spanContext(); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: eventId, + sent_at: expect.any(String), + trace: { + trace_id: traceId, + environment: 'production', + public_key: 'dsn', + sampled: 'true', + sample_rate: '1', + transaction: 'test-span', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + trace_id: traceId, + span_id: spanId, + data: { + 'sentry.origin': 'manual', + 'sentry.sample_rate': 1, + 'sentry.source': 'custom', + }, + origin: 'manual', + }, + feedback: { + message: 'test', + }, + }, + level: 'info', + environment: 'production', + event_id: eventId, + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); +}); diff --git a/packages/core/test/mocks/client.ts b/packages/core/test/mocks/client.ts index 473028ea4b12..8063dab46592 100644 --- a/packages/core/test/mocks/client.ts +++ b/packages/core/test/mocks/client.ts @@ -77,13 +77,14 @@ export class TestClient extends BaseClient { public sendEvent(event: Event, hint?: EventHint): void { this.event = event; - // In real life, this will get deleted as part of envelope creation. - delete event.sdkProcessingMetadata; - if (this._options.enableSend) { super.sendEvent(event, hint); return; } + + // In real life, this will get deleted as part of envelope creation. + delete event.sdkProcessingMetadata; + TestClient.sendEventCalled && TestClient.sendEventCalled(event); } diff --git a/packages/feedback/src/core/sendFeedback.test.ts b/packages/feedback/src/core/sendFeedback.test.ts index 2b139874783f..a59054f278a3 100644 --- a/packages/feedback/src/core/sendFeedback.test.ts +++ b/packages/feedback/src/core/sendFeedback.test.ts @@ -30,7 +30,58 @@ describe('sendFeedback', () => { patchedDecoder && delete global.window.TextDecoder; }); - it('sends feedback', async () => { + it('sends feedback with minimal options', async () => { + mockSdk(); + const mockTransport = jest.spyOn(getClient()!.getTransport()!, 'send'); + + const promise = sendFeedback({ + message: 'mi', + }); + + expect(promise).toBeInstanceOf(Promise); + + const eventId = await promise; + + expect(typeof eventId).toEqual('string'); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: expect.any(String), + sent_at: expect.any(String), + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'dsn', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: undefined, + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + message: 'mi', + source: 'api', + url: 'http://localhost/', + }, + }, + level: 'info', + environment: 'production', + event_id: expect.any(String), + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); + + it('sends feedback with full options', async () => { mockSdk(); const mockTransport = jest.spyOn(getClient()!.getTransport()!, 'send'); @@ -38,6 +89,9 @@ describe('sendFeedback', () => { name: 'doe', email: 're@example.org', message: 'mi', + url: 'http://example.com/', + source: 'custom-source', + associatedEventId: '1234', }); expect(promise).toBeInstanceOf(Promise); @@ -67,11 +121,12 @@ describe('sendFeedback', () => { trace_id: expect.any(String), }, feedback: { + name: 'doe', contact_email: 're@example.org', message: 'mi', - name: 'doe', - source: 'api', - url: 'http://localhost/', + url: 'http://example.com/', + source: 'custom-source', + associated_event_id: '1234', }, }, level: 'info', diff --git a/packages/feedback/src/core/sendFeedback.ts b/packages/feedback/src/core/sendFeedback.ts index 3d99abfb5a70..7feff8b98e1e 100644 --- a/packages/feedback/src/core/sendFeedback.ts +++ b/packages/feedback/src/core/sendFeedback.ts @@ -9,10 +9,10 @@ import { FEEDBACK_API_SOURCE } from '../constants'; * Public API to send a Feedback item to Sentry */ export const sendFeedback: SendFeedback = ( - { name, email, message, attachments, source = FEEDBACK_API_SOURCE, url = getLocationHref() }: SendFeedbackParams, + options: SendFeedbackParams, { includeReplay = true } = {}, ): Promise => { - if (!message) { + if (!options.message) { throw new Error('Unable to submit feedback with empty message'); } @@ -23,7 +23,14 @@ export const sendFeedback: SendFeedback = ( throw new Error('No client setup, cannot send feedback.'); } - const eventId = captureFeedback({ name, email, message, attachments, source, url }, { includeReplay }); + const eventId = captureFeedback( + { + source: FEEDBACK_API_SOURCE, + url: getLocationHref(), + ...options, + }, + { includeReplay }, + ); // We want to wait for the feedback to be sent (or not) return new Promise((resolve, reject) => { diff --git a/packages/types/src/feedback/sendFeedback.ts b/packages/types/src/feedback/sendFeedback.ts index 1dc0d6fa4369..3ea126eff693 100644 --- a/packages/types/src/feedback/sendFeedback.ts +++ b/packages/types/src/feedback/sendFeedback.ts @@ -39,6 +39,7 @@ export interface SendFeedbackParams { attachments?: Attachment[]; url?: string; source?: string; + associatedEventId?: string; } interface SendFeedbackOptions { From 3dde93a19d656786a619e4e289f1f6c2a6feecdb Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 19 Apr 2024 15:02:37 +0200 Subject: [PATCH 07/11] one more test --- packages/core/test/lib/feedback.test.ts | 84 ++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/packages/core/test/lib/feedback.test.ts b/packages/core/test/lib/feedback.test.ts index 563fc812a138..7b209f886e06 100644 --- a/packages/core/test/lib/feedback.test.ts +++ b/packages/core/test/lib/feedback.test.ts @@ -1,5 +1,5 @@ -import { Span } from '@sentry/types'; -import { getCurrentScope, setCurrentClient, startSpan } from '../../src'; +import type { Span } from '@sentry/types'; +import { addBreadcrumb, getCurrentScope, setCurrentClient, startSpan, withIsolationScope, withScope } from '../../src'; import { captureFeedback } from '../../src/feedback'; import { TestClient, getDefaultTestClientOptions } from '../mocks/client'; @@ -380,4 +380,84 @@ describe('captureFeedback', () => { ], ]); }); + + it('applies scope data to feedback', async () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + enableSend: true, + enableTracing: true, + // We don't care about transactions here... + beforeSendTransaction() { + return null; + }, + }), + ); + setCurrentClient(client); + client.init(); + + const mockTransport = jest.spyOn(client.getTransport()!, 'send'); + + withIsolationScope(isolationScope => { + isolationScope.setTag('test-1', 'tag'); + isolationScope.setExtra('test-1', 'extra'); + + return withScope(scope => { + scope.setTag('test-2', 'tag'); + scope.setExtra('test-2', 'extra'); + + addBreadcrumb({ message: 'test breadcrumb', timestamp: 12345 }); + + captureFeedback({ + name: 'doe', + email: 're@example.org', + message: 'mi', + }); + }); + }); + + expect(mockTransport).toHaveBeenCalledWith([ + { + event_id: expect.any(String), + sent_at: expect.any(String), + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'dsn', + }, + }, + [ + [ + { type: 'feedback' }, + { + breadcrumbs: [{ message: 'test breadcrumb', timestamp: 12345 }], + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + }, + feedback: { + contact_email: 're@example.org', + message: 'mi', + name: 'doe', + }, + }, + extra: { + 'test-1': 'extra', + 'test-2': 'extra', + }, + tags: { + 'test-1': 'tag', + 'test-2': 'tag', + }, + level: 'info', + environment: 'production', + event_id: expect.any(String), + timestamp: expect.any(Number), + type: 'feedback', + }, + ], + ], + ]); + }); }); From ba576aebe924c28cbf91441b4d963e29fb7f0e28 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 26 Apr 2024 13:53:51 +0200 Subject: [PATCH 08/11] attachments in single envelope --- .../trace-lifetime/pageload-meta/test.ts | 2 +- .../tracing/trace-lifetime/pageload/test.ts | 2 +- packages/core/src/envelope.ts | 31 ----------- packages/core/src/feedback.ts | 27 +-------- packages/core/src/index.ts | 2 +- packages/core/test/lib/feedback.test.ts | 52 +++++++----------- .../feedback/src/core/sendFeedback.test.ts | 55 +++++++------------ packages/feedback/src/core/sendFeedback.ts | 6 +- .../feedback/src/modal/components/Form.tsx | 13 ++++- packages/types/src/feedback/sendFeedback.ts | 6 +- 10 files changed, 63 insertions(+), 133 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index 39a1ea92ef8e..5d2d837c369b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { SpanEnvelope, Event } from '@sentry/types'; +import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 6c23fc643a83..b69127cf8017 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { SpanEnvelope, Event } from '@sentry/types'; +import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 11d38b2040e6..2aef194b069e 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -1,6 +1,4 @@ import type { - Attachment, - AttachmentItem, DsnComponents, DynamicSamplingContext, Event, @@ -15,7 +13,6 @@ import type { SpanEnvelope, } from '@sentry/types'; import { - createAttachmentEnvelopeItem, createEnvelope, createEventEnvelopeHeaders, dsnToString, @@ -95,34 +92,6 @@ export function createEventEnvelope( return createEnvelope(envelopeHeaders, [eventItem]); } -/** - * Create an Envelope from an event. - */ -export function createAttachmentEnvelope( - event: Event, - attachments: Attachment[], - dsn?: DsnComponents, - metadata?: SdkMetadata, - tunnel?: string, -): EventEnvelope { - const sdkInfo = getSdkMetadataForEnvelopeHeader(metadata); - enhanceEventWithSdkInfo(event, metadata && metadata.sdk); - - const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn); - - // Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to - // sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may - // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid - // of this `delete`, lest we miss putting it back in the next time the property is in use.) - delete event.sdkProcessingMetadata; - - const attachmentItems: AttachmentItem[] = []; - for (const attachment of attachments || []) { - attachmentItems.push(createAttachmentEnvelopeItem(attachment)); - } - return createEnvelope(envelopeHeaders, attachmentItems); -} - /** * Create envelope from Span item. */ diff --git a/packages/core/src/feedback.ts b/packages/core/src/feedback.ts index ac78efeaa319..ae3abc7ca50f 100644 --- a/packages/core/src/feedback.ts +++ b/packages/core/src/feedback.ts @@ -1,20 +1,17 @@ import type { EventHint, FeedbackEvent, SendFeedbackParams } from '@sentry/types'; import { dropUndefinedKeys } from '@sentry/utils'; import { getClient, getCurrentScope } from './currentScopes'; -import { createAttachmentEnvelope } from './envelope'; /** * Send user feedback to Sentry. */ export function captureFeedback( feedbackParams: SendFeedbackParams, - hint?: EventHint & { includeReplay?: boolean }, + hint: EventHint & { includeReplay?: boolean } = {}, ): string { - const { message, name, email, url, source, attachments, associatedEventId } = feedbackParams; + const { message, name, email, url, source, associatedEventId } = feedbackParams; const client = getClient(); - const transport = client && client.getTransport(); - const dsn = client && client.getDsn(); const feedbackEvent: FeedbackEvent = { contexts: { @@ -37,25 +34,5 @@ export function captureFeedback( const eventId = getCurrentScope().captureEvent(feedbackEvent, hint); - // For now, we have to send attachments manually in a separate envelope - // Because we do not support attachments in the feedback envelope - // Once the Sentry API properly supports this, we can get rid of this and send it through the event envelope - if (client && transport && dsn && attachments && attachments.length) { - // TODO: https://docs.sentry.io/platforms/javascript/enriching-events/attachments/ - // eslint-disable-next-line @typescript-eslint/no-floating-promises - void transport.send( - createAttachmentEnvelope( - { - ...feedbackEvent, - event_id: eventId, - }, - attachments, - dsn, - client.getOptions()._metadata, - client.getOptions().tunnel, - ), - ); - } - return eventId; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c781d4f10d46..743a2e9d9bd8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -8,7 +8,7 @@ export type { IntegrationIndex } from './integration'; export * from './tracing'; export * from './semanticAttributes'; -export { createEventEnvelope, createSessionEnvelope, createAttachmentEnvelope, createSpanEnvelope } from './envelope'; +export { createEventEnvelope, createSessionEnvelope, createSpanEnvelope } from './envelope'; export { captureCheckIn, withMonitor, diff --git a/packages/core/test/lib/feedback.test.ts b/packages/core/test/lib/feedback.test.ts index 7b209f886e06..faa17a8c51ea 100644 --- a/packages/core/test/lib/feedback.test.ts +++ b/packages/core/test/lib/feedback.test.ts @@ -152,29 +152,33 @@ describe('captureFeedback', () => { const attachment1 = new Uint8Array([1, 2, 3, 4, 5]); const attachment2 = new Uint8Array([6, 7, 8, 9]); - const eventId = captureFeedback({ - message: 'test', - attachments: [ - { - data: attachment1, - filename: 'test-file.txt', - }, - { - data: attachment2, - filename: 'test-file2.txt', - }, - ], - }); + const eventId = captureFeedback( + { + message: 'test', + }, + { + attachments: [ + { + data: attachment1, + filename: 'test-file.txt', + }, + { + data: attachment2, + filename: 'test-file2.txt', + }, + ], + }, + ); await client.flush(); expect(typeof eventId).toBe('string'); - expect(mockTransport).toHaveBeenCalledTimes(2); + expect(mockTransport).toHaveBeenCalledTimes(1); - const [feedbackEnvelope, attachmentEnvelope] = mockTransport.mock.calls; + const [feedbackEnvelope] = mockTransport.mock.calls; - // Feedback event is sent normally in one envelope + expect(feedbackEnvelope).toHaveLength(1); expect(feedbackEnvelope[0]).toEqual([ { event_id: eventId, @@ -206,16 +210,6 @@ describe('captureFeedback', () => { type: 'feedback', }, ], - ], - ]); - - // Attachments are sent in separate envelope - expect(attachmentEnvelope[0]).toEqual([ - { - event_id: eventId, - sent_at: expect.any(String), - }, - [ [ { type: 'attachment', @@ -359,12 +353,6 @@ describe('captureFeedback', () => { trace: { trace_id: traceId, span_id: spanId, - data: { - 'sentry.origin': 'manual', - 'sentry.sample_rate': 1, - 'sentry.source': 'custom', - }, - origin: 'manual', }, feedback: { message: 'test', diff --git a/packages/feedback/src/core/sendFeedback.test.ts b/packages/feedback/src/core/sendFeedback.test.ts index a59054f278a3..e87573533952 100644 --- a/packages/feedback/src/core/sendFeedback.test.ts +++ b/packages/feedback/src/core/sendFeedback.test.ts @@ -174,12 +174,6 @@ describe('sendFeedback', () => { trace: { span_id: expect.any(String), trace_id: expect.any(String), - data: { - 'sentry.origin': 'manual', - 'sentry.sample_rate': 1, - 'sentry.source': 'custom', - }, - origin: 'manual', }, feedback: { contact_email: 're@example.org', @@ -344,32 +338,35 @@ describe('sendFeedback', () => { const attachment1 = new Uint8Array([1, 2, 3, 4, 5]); const attachment2 = new Uint8Array([6, 7, 8, 9]); - const promise = sendFeedback({ - name: 'doe', - email: 're@example.org', - message: 'mi', - attachments: [ - { - data: attachment1, - filename: 'test-file.txt', - }, - { - data: attachment2, - filename: 'test-file2.txt', - }, - ], - }); + const promise = sendFeedback( + { + name: 'doe', + email: 're@example.org', + message: 'mi', + }, + { + attachments: [ + { + data: attachment1, + filename: 'test-file.txt', + }, + { + data: attachment2, + filename: 'test-file2.txt', + }, + ], + }, + ); expect(promise).toBeInstanceOf(Promise); const eventId = await promise; expect(typeof eventId).toEqual('string'); - expect(mockTransport).toHaveBeenCalledTimes(2); + expect(mockTransport).toHaveBeenCalledTimes(1); - const [feedbackEnvelope, attachmentEnvelope] = mockTransport.mock.calls; + const [feedbackEnvelope] = mockTransport.mock.calls; - // Feedback event is sent normally in one envelope expect(feedbackEnvelope[0]).toEqual([ { event_id: eventId, @@ -405,16 +402,6 @@ describe('sendFeedback', () => { type: 'feedback', }, ], - ], - ]); - - // Attachments are sent in separate envelope - expect(attachmentEnvelope[0]).toEqual([ - { - event_id: eventId, - sent_at: expect.any(String), - }, - [ [ { type: 'attachment', diff --git a/packages/feedback/src/core/sendFeedback.ts b/packages/feedback/src/core/sendFeedback.ts index 7feff8b98e1e..3848eb9cbc33 100644 --- a/packages/feedback/src/core/sendFeedback.ts +++ b/packages/feedback/src/core/sendFeedback.ts @@ -1,6 +1,6 @@ import { captureFeedback } from '@sentry/core'; import { getClient } from '@sentry/core'; -import type { SendFeedback, SendFeedbackParams, TransportMakeRequestResponse } from '@sentry/types'; +import type { EventHint, SendFeedback, SendFeedbackParams, TransportMakeRequestResponse } from '@sentry/types'; import type { Event } from '@sentry/types'; import { getLocationHref } from '@sentry/utils'; import { FEEDBACK_API_SOURCE } from '../constants'; @@ -10,7 +10,7 @@ import { FEEDBACK_API_SOURCE } from '../constants'; */ export const sendFeedback: SendFeedback = ( options: SendFeedbackParams, - { includeReplay = true } = {}, + hint: EventHint & { includeReplay?: boolean } = { includeReplay: true }, ): Promise => { if (!options.message) { throw new Error('Unable to submit feedback with empty message'); @@ -29,7 +29,7 @@ export const sendFeedback: SendFeedback = ( url: getLocationHref(), ...options, }, - { includeReplay }, + hint, ); // We want to wait for the feedback to be sent (or not) diff --git a/packages/feedback/src/modal/components/Form.tsx b/packages/feedback/src/modal/components/Form.tsx index f40693d8f2af..3a723bc61d1b 100644 --- a/packages/feedback/src/modal/components/Form.tsx +++ b/packages/feedback/src/modal/components/Form.tsx @@ -112,17 +112,28 @@ export function Form({ } const formData = new FormData(e.target); const attachment = await (screenshotInput && showScreenshotInput ? screenshotInput.value() : undefined); + const data: FeedbackFormData = { name: retrieveStringValue(formData, 'name'), email: retrieveStringValue(formData, 'email'), message: retrieveStringValue(formData, 'message'), attachments: attachment ? [attachment] : undefined, }; + if (!hasAllRequiredFields(data)) { return; } + try { - await onSubmit({ ...data, source: FEEDBACK_WIDGET_SOURCE }); + await onSubmit( + { + name: data.name, + email: data.email, + message: data.message, + source: FEEDBACK_WIDGET_SOURCE, + }, + { attachments: data.attachments }, + ); onSubmitSuccess(data); } catch (error) { DEBUG_BUILD && logger.error(error); diff --git a/packages/types/src/feedback/sendFeedback.ts b/packages/types/src/feedback/sendFeedback.ts index 3ea126eff693..a284e82f107b 100644 --- a/packages/types/src/feedback/sendFeedback.ts +++ b/packages/types/src/feedback/sendFeedback.ts @@ -1,5 +1,4 @@ -import type { Attachment } from '../attachment'; -import type { Event } from '../event'; +import type { Event, EventHint } from '../event'; import type { User } from '../user'; /** @@ -36,13 +35,12 @@ export interface SendFeedbackParams { message: string; name?: string; email?: string; - attachments?: Attachment[]; url?: string; source?: string; associatedEventId?: string; } -interface SendFeedbackOptions { +interface SendFeedbackOptions extends EventHint { /** * Should include replay with the feedback? */ From 5c79ebdffb62b98bb554530a9d850f7609a0bc9a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 30 Apr 2024 10:22:57 +0200 Subject: [PATCH 09/11] skip tests correctly --- .../suites/tracing/trace-lifetime/navigation/test.ts | 4 ++-- .../suites/tracing/trace-lifetime/pageload-meta/test.ts | 4 ++-- .../suites/tracing/trace-lifetime/pageload/test.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index bb0e6d7255fc..ea3b82fa730e 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -1,7 +1,7 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import type { EventAndTraceHeader } from '../../../../utils/helpers'; +import { EventAndTraceHeader, shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, getFirstSentryEnvelopeRequest, @@ -460,7 +460,7 @@ sentryTest( sentryTest( 'user feedback event after navigation has navigation traceId in headers', async ({ getLocalTestPath, page }) => { - if (shouldSkipTracingTest()) { + if (shouldSkipTracingTest() || shouldSkipFeedbackTest()) { sentryTest.skip(); } diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index 5d2d837c369b..6b3da62e3481 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -1,7 +1,7 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import type { EventAndTraceHeader } from '../../../../utils/helpers'; +import { EventAndTraceHeader, shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, getFirstSentryEnvelopeRequest, @@ -431,7 +431,7 @@ sentryTest( ); sentryTest('user feedback event after pageload has pageload traceId in headers', async ({ getLocalTestPath, page }) => { - if (shouldSkipTracingTest()) { + if (shouldSkipTracingTest() || shouldSkipFeedbackTest()) { sentryTest.skip(); } diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index b69127cf8017..128bdbb8c47b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -1,7 +1,7 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import type { EventAndTraceHeader } from '../../../../utils/helpers'; +import { EventAndTraceHeader, shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, getFirstSentryEnvelopeRequest, @@ -433,7 +433,7 @@ sentryTest( ); sentryTest('user feedback event after pageload has pageload traceId in headers', async ({ getLocalTestPath, page }) => { - if (shouldSkipTracingTest()) { + if (shouldSkipTracingTest() || shouldSkipFeedbackTest()) { sentryTest.skip(); } From 5b82f1a2b672240e0d100114d255a9521b65ca8e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 30 Apr 2024 11:39:53 +0200 Subject: [PATCH 10/11] fix lint --- .../suites/tracing/trace-lifetime/navigation/test.ts | 3 ++- .../suites/tracing/trace-lifetime/pageload-meta/test.ts | 3 ++- .../suites/tracing/trace-lifetime/pageload/test.ts | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index ea3b82fa730e..500ee16ad313 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -1,7 +1,8 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import { EventAndTraceHeader, shouldSkipFeedbackTest } from '../../../../utils/helpers'; +import type { EventAndTraceHeader} from '../../../../utils/helpers'; +import { shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, getFirstSentryEnvelopeRequest, diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index 6b3da62e3481..5e5c5a4b8d3c 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -1,7 +1,8 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import { EventAndTraceHeader, shouldSkipFeedbackTest } from '../../../../utils/helpers'; +import type { EventAndTraceHeader} from '../../../../utils/helpers'; +import { shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, getFirstSentryEnvelopeRequest, diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 128bdbb8c47b..3b3fa0039382 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -1,7 +1,8 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import { EventAndTraceHeader, shouldSkipFeedbackTest } from '../../../../utils/helpers'; +import type { EventAndTraceHeader} from '../../../../utils/helpers'; +import { shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, getFirstSentryEnvelopeRequest, From f14d0c835a325af555af6fa31a00c45206ecbb6a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 30 Apr 2024 12:21:40 +0200 Subject: [PATCH 11/11] fix format... --- .../suites/tracing/trace-lifetime/navigation/test.ts | 2 +- .../suites/tracing/trace-lifetime/pageload-meta/test.ts | 2 +- .../suites/tracing/trace-lifetime/pageload/test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index 500ee16ad313..a05f0da3d91d 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -1,7 +1,7 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import type { EventAndTraceHeader} from '../../../../utils/helpers'; +import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index 5e5c5a4b8d3c..78582a8369c1 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -1,7 +1,7 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import type { EventAndTraceHeader} from '../../../../utils/helpers'; +import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser, diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 3b3fa0039382..3b6a007c54fb 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -1,7 +1,7 @@ import { expect } from '@playwright/test'; import type { Event, SpanEnvelope } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import type { EventAndTraceHeader} from '../../../../utils/helpers'; +import type { EventAndTraceHeader } from '../../../../utils/helpers'; import { shouldSkipFeedbackTest } from '../../../../utils/helpers'; import { eventAndTraceHeaderRequestParser,