Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(browser): Add captureUserFeedback #7729

Merged
merged 10 commits into from
Apr 5, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Sentry.captureUserFeedback({
eventId: 'test_event_id',
email: 'test_email',
comments: 'test_comments',
name: 'test_name',
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { expect } from '@playwright/test';
import type { UserFeedback } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';

sentryTest('should capture simple user feedback', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<UserFeedback>(page, url);

expect(eventData).toMatchObject({
eventId: 'test_event_id',
email: 'test_email',
comments: 'test_comments',
name: 'test_name',
});
});
1 change: 1 addition & 0 deletions packages/browser/src/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,5 @@ export {
} from './stack-parsers';
export { eventFromException, eventFromMessage } from './eventbuilder';
export { defaultIntegrations, forceLoad, init, lastEventId, onLoad, showReportDialog, flush, close, wrap } from './sdk';
export { captureUserFeedback } from './userfeedback';
export { GlobalHandlers, TryCatch, Breadcrumbs, LinkedErrors, HttpContext, Dedupe } from './integrations';
70 changes: 70 additions & 0 deletions packages/browser/src/userfeedback.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { getCurrentHub } from '@sentry/core';
import type { DsnComponents, EventEnvelope, SdkMetadata, UserFeedback, UserFeedbackItem } from '@sentry/types';
import { createEnvelope, dsnToString, logger } from '@sentry/utils';

/**
* Sends user feedback to Sentry.
*/
export function captureUserFeedback(feedback: UserFeedback): void {
const hub = getCurrentHub();
const client = hub.getClient();
const transport = client && client.getTransport();

if (!client) {
__DEBUG_BUILD__ && logger.log('[UserFeedback] getClient did not return a Client, user feedback will not be sent.');
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (!transport) {
__DEBUG_BUILD__ &&
logger.log('[UserFeedback] getTransport did not return a Transport, user feedback will not be sent.');
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
return;
}

const envelope = createUserFeedbackEnvelope(feedback, {
metadata: client.getSdkMetadata && client.getSdkMetadata(),
dsn: client.getDsn(),
tunnel: client.getOptions().tunnel,
});

void transport.send(envelope);
Copy link
Member

@AbhiPrasad AbhiPrasad Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's times like this I wish client.sendEnvelope was exposed - @lforst do you think we should just expose it?

m: We can only call transport.send if client.getOptions().enabled !== false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: We can only call transport.send if client.getOptions().enabled !== false

I have a feeling this should be checked in a more central location. Doesn't have to be this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's times like this I wish client.sendEnvelope was exposed - @lforst do you think we should just expose it?

Probably? It seems safer that this whole chain of grabbing stuff off the hub that may or may not be there. But honestly sendEnvelope seems a bit too low-level to just expose it on a pretty public place.

I vote we take the bundle size hit and go ahead and implement user feedback like we implemented sessions, errors, and transactions just to stay consistent - meaning we put it as a captureUserFeedback() method on the (browser-)client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me @krystofwoldrich - sorry for making you revert your work, but let's just add the methods to the browser client.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind either, but the stand-alone function is a bit easier for reuse in other JS SDKs,

if we implement it on the Browser client we have to export the user feedback envelope creator for other JS-based SDKs,

if we would implement it as a standalone function other JS SDKs can use the method right away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both electron and react native wrap browser - so it's only the node SDKs that won't have access to this, and I think that is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's fair not that many SDKs, just a note RN doesn't wrap Browser.

}

/**
* Creates an envelope from a user feedback.
*/
export function createUserFeedbackEnvelope(
feedback: UserFeedback,
{
metadata,
tunnel,
dsn,
}: {
metadata: SdkMetadata | undefined;
tunnel: string | undefined;
dsn: DsnComponents | undefined;
},
): EventEnvelope {
const headers: EventEnvelope[0] = {
event_id: feedback.event_id,
sent_at: new Date().toISOString(),
...(metadata &&
metadata.sdk && {
sdk: {
name: metadata.sdk.name,
version: metadata.sdk.version,
},
}),
...(!!tunnel && !!dsn && { dsn: dsnToString(dsn) }),
};
const item = createUserFeedbackEnvelopeItem(feedback);

return createEnvelope(headers, [item]);
}

function createUserFeedbackEnvelopeItem(feedback: UserFeedback): UserFeedbackItem {
const feedbackHeaders: UserFeedbackItem[0] = {
type: 'user_report',
};
return [feedbackHeaders, feedback];
}
68 changes: 68 additions & 0 deletions packages/browser/test/unit/userfeedback.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { createUserFeedbackEnvelope } from '../../src/userfeedback';

describe('userFeedback', () => {
test('creates user feedback envelope header', () => {
const envelope = createUserFeedbackEnvelope(
{
comments: 'Test Comments',
email: 'test@email.com',
name: 'Test User',
event_id: 'testEvent123',
},
{
metadata: {
sdk: {
name: 'testSdkName',
version: 'testSdkVersion',
},
},
tunnel: 'testTunnel',
dsn: {
host: 'testHost',
projectId: 'testProjectId',
protocol: 'http',
},
},
);

expect(envelope[0]).toEqual({
dsn: 'http://undefined@testHost/undefinedtestProjectId',
event_id: 'testEvent123',
sdk: {
name: 'testSdkName',
version: 'testSdkVersion',
},
sent_at: expect.any(String),
});
});

test('creates user feedback envelope item', () => {
const envelope = createUserFeedbackEnvelope(
{
comments: 'Test Comments',
email: 'test@email.com',
name: 'Test User',
event_id: 'testEvent123',
},
{
metadata: undefined,
tunnel: undefined,
dsn: undefined,
},
);

expect(envelope[1]).toEqual([
[
{
type: 'user_report',
},
{
comments: 'Test Comments',
email: 'test@email.com',
name: 'Test User',
event_id: 'testEvent123',
},
],
]);
});
});