From 743ab6d273e6c2b898509307135523f26de2b2e9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 26 Apr 2024 13:53:51 +0200 Subject: [PATCH] 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 b68d0df43855..ffaa0b8cac89 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? */