feat(feedback): Add captureFeedback method (#11428)

This PR adds a new `captureFeedback` method which is exported from all SDKs. This method can be used to capture a (new) user feedback from any package. We follow the same semantics as for other capture methods like `captureException()` or `captureMessage()`: The method returns a string, which is the event id. We do not wait for sending to be successful or not, we just try to send it and return. You can both set an `associatedEventId` (which replaces the event_id of the "old" `captureUserFeedback`), and also pass attachments to send along (which for now are sent as a separate envelope). For usage in the modal UI, there is still `sendFeedback` which continues to return a promise that resolves with the event ID, or rejects with a meaningful error message if sending fails. This also deprecates `captureUserFeedback()`, which is only exported in browser. We cannot remove this yet because `captureFeedback` will only work on newer self-hosted instances, so not all users can easily update. We can/should remove this in v9. Includes #11626 Fixes 49946
getsentry · May 3, 2024 · 56197af · 56197af
1 parent 85c754a
commit 56197af
Show file tree

Hide file tree

Showing 32 changed files with 1,094 additions and 174 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
@@ -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/...es/browser-integration-tests/suites/feedback/captureFeedbackAndReplay/hasSampling/test.ts b/...es/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/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
@@ -2,6 +2,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 { shouldSkipFeedbackTest } from '../../../../utils/helpers';
 import {
   eventAndTraceHeaderRequestParser,
   getFirstSentryEnvelopeRequest,
@@ -134,7 +135,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<Event>(page, url);
 
   const envelopeRequestsPromise = getMultipleSentryEnvelopeRequests<EventAndTraceHeader>(
@@ -202,7 +203,7 @@ sentryTest(
       });
     });
 
-    // ensure navigation transaction is finished
+    // ensure pageload transaction is finished
     await getFirstSentryEnvelopeRequest<Event>(page, url);
 
     const [navigationEvent, navigationTraceHeader] = await getFirstSentryEnvelopeRequest<EventAndTraceHeader>(
@@ -276,7 +277,7 @@ sentryTest(
       });
     });
 
-    // ensure navigation transaction is finished
+    // ensure pageload transaction is finished
     await getFirstSentryEnvelopeRequest<Event>(page, url);
 
     const navigationEventPromise = getFirstSentryEnvelopeRequest<EventAndTraceHeader>(
@@ -456,3 +457,47 @@ sentryTest(
     );
   },
 );
+
+sentryTest(
+  'user feedback event after navigation has navigation traceId in headers',
+  async ({ getLocalTestPath, page }) => {
+    if (shouldSkipTracingTest() || shouldSkipFeedbackTest()) {
+      sentryTest.skip();
+    }
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    // ensure pageload transaction is finished
+    await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+    const navigationEvent = await getFirstSentryEnvelopeRequest<Event>(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<Event>(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;
+
+    expect(feedbackEvent.type).toEqual('feedback');
+
+    const feedbackTraceContext = feedbackEvent.contexts?.trace;
+
+    expect(feedbackTraceContext).toMatchObject({
+      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
@@ -1,7 +1,8 @@
 import { expect } from '@playwright/test';
-import type { SpanEnvelope } from '@sentry/types';
+import type { Event, SpanEnvelope } from '@sentry/types';
 import { sentryTest } from '../../../../utils/fixtures';
 import type { EventAndTraceHeader } from '../../../../utils/helpers';
+import { shouldSkipFeedbackTest } from '../../../../utils/helpers';
 import {
   eventAndTraceHeaderRequestParser,
   getFirstSentryEnvelopeRequest,
@@ -429,3 +430,41 @@ sentryTest(
     expect(headers['baggage']).toBe(META_TAG_BAGGAGE);
   },
 );
+
+sentryTest('user feedback event after pageload has pageload traceId in headers', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipTracingTest() || shouldSkipFeedbackTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+
+  const pageloadEvent = await getFirstSentryEnvelopeRequest<Event>(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<Event>(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(feedbackEvent.type).toEqual('feedback');
+
+  expect(feedbackTraceContext).toMatchObject({
+    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
@@ -1,7 +1,8 @@
 import { expect } from '@playwright/test';
-import type { SpanEnvelope } from '@sentry/types';
+import type { Event, SpanEnvelope } from '@sentry/types';
 import { sentryTest } from '../../../../utils/fixtures';
 import type { EventAndTraceHeader } from '../../../../utils/helpers';
+import { shouldSkipFeedbackTest } from '../../../../utils/helpers';
 import {
   eventAndTraceHeaderRequestParser,
   getFirstSentryEnvelopeRequest,
@@ -431,3 +432,41 @@ sentryTest(
     );
   },
 );
+
+sentryTest('user feedback event after pageload has pageload traceId in headers', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipTracingTest() || shouldSkipFeedbackTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+
+  const pageloadEvent = await getFirstSentryEnvelopeRequest<Event>(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<Event>(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;
+
+  expect(feedbackEvent.type).toEqual('feedback');
+
+  const feedbackTraceContext = feedbackEvent.contexts?.trace;
+
+  expect(feedbackTraceContext).toMatchObject({
+    trace_id: pageloadTraceContext?.trace_id,
+    span_id: expect.stringMatching(/^[0-9a-f]{16}$/),
+  });
+});
diff --git 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
@@ -6,6 +6,7 @@ export {
   captureEvent,
   captureMessage,
   captureCheckIn,
+  captureFeedback,
   startSession,
   captureSession,
   endSession,

diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts
@@ -91,6 +91,8 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
 
   /**
    * 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<BrowserClient>();
   if (client) {
+    // eslint-disable-next-line deprecation/deprecation
     client.captureUserFeedback(feedback);
   }
 }
diff --git 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/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<O extends ClientOptions> implements Client<O> {
       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/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<EventEnvelope>(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<EventEnvelope>(envelopeHeaders, attachmentItems);
-}
-
 /**
  * Create envelope from Span item.
  */