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

mydea · 2024-04-04T14:14:02Z

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

github-actions · 2024-04-04T14:24:02Z

size-limit report 📦

Path	Size
@sentry/browser	21.65 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing)	32.69 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay)	68.04 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.43 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)	72.07 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)	84.2 KB (-0.09% 🔽)
@sentry/browser (incl. Feedback)	37.69 KB (-0.39% 🔽)
@sentry/browser (incl. sendFeedback)	26.21 KB (-0.87% 🔽)
@sentry/browser (incl. FeedbackAsync)	30.77 KB (-0.65% 🔽)
@sentry/react	24.33 KB (+0.01% 🔺)
@sentry/react (incl. Tracing)	35.65 KB (+0.01% 🔺)
@sentry/vue	25.48 KB (+0.01% 🔺)
@sentry/vue (incl. Tracing)	34.48 KB (+0.01% 🔺)
@sentry/svelte	21.78 KB (+0.01% 🔺)
CDN Bundle	24.03 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing)	34.05 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay)	67.73 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)	72.85 KB (-0.3% 🔽)
CDN Bundle - uncompressed	70.64 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing) - uncompressed	101 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed	210.61 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	223.66 KB (-0.26% 🔽)
@sentry/nextjs (client)	34.87 KB (+0.01% 🔺)
@sentry/sveltekit (client)	33.25 KB (+0.01% 🔺)
@sentry/node	138.72 KB (+0.01% 🔺)

Lms24 · 2024-04-16T12:22:02Z

Feel free to merge #11626 into this PR if it makes sense; otherwise we can also merge sequentially :)

Lms24

LGTM!

billyvg

LGTM! Thanks for the tests

ryan953 · 2024-04-19T17:23:31Z

packages/feedback/src/core/sendFeedback.ts

 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
 */
 export const sendFeedback: SendFeedback = (


Doesn't have to be in this PR, but should we continue to export this publicly? Or just leave it as an internal helper for our own UI?

that's a good question, I don't have a strong feeling there! I think for backwards compatibility we might as well continue to export it, but I'll defer to whatever you folks think about this 🤔

I think with the new captureFeedback() being a thing, this isn't too important for people anymore. If we were going to remove it now would be a fine time.

It seems like if we keep it, then the docs will be something like:

There is captureFeedback(), and also sendFeedback()... the differences are....

OR if we remove it, docs could instead mention:

There is captureFeedback(). You can response by doing something like client.on('afterSendEvent', (event, response) => ...) and check that event.event_id === eventId

I'm starting a migration doc to track moving from "feedback beta & v7" into "v8 & GA", so this could fit there too.
#11731

I'll remove the public export then in a follow up PR 👍

ryan953 · 2024-04-19T19:18:47Z

packages/core/src/feedback.ts

+  // 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) {


@cmanallen @aliu3ntry Is this still true?

You can send whatever you want in an envelope. You're just capped to three items. At least, as far as I know.

I think Andrew has a PR up but it's not merged yet

Yes..I opened a relay PR to handle this that's still a work in progress

I'm confused on how that PR solves the problem? Are you trying to put attachments into the feedback envelope item? Or are you trying to send attachments in the same envelope with a feedback item? These are two different problems.

Let's talk about this on Monday before my PTO.

See https://github.com/getsentry/team-replay/issues/393

I discussed this with Andrew, but we want to send attachments in the same envelope as the feedback, the attachment will be its own item. We want to use getCurrentScope().addAttachment() to add the attachment to the envelope.

OK, to clarify: This means I will remove the code to add attachments to a separate envelope, and also remove the attachments option for the method, instead expecting this to be added to the scope the same way as for any other event? --> This will reduce bundle size and complexity here too, because this should be handled OOTB by captureEvent so this should "just work" really.

This is OK and will work (with up-to-date self-hosted, at least) today?

It looks like https://github.com/getsentry/team-replay/issues/393 hasn't landed yet; but @mydea the SDK should send attachments and feedback together IMO.
Why?

Sending them in the same payload gives us better guarantees that both parts made it successfully, so we won't get into cases where random attachments are ingested, but no matching feedback exists.

We'll save a bunch of SDK code

We can take advantage of being in alpha & beta state for the screenshot feature specifically.... I think it's ok to have a higher min-requirement on the server because turning on screenshots is opt-in right now.

For example, in the docs we could say something like (this is the idea, not the verbatim docs i'd write):

v8 of the Feedback SDK supports allowing users to include a screenshot with their feedback submission.
On-Prem users must update to version ${after getsentry/team-replay#393 lands} before enabling showScreenshot: true in the SDK config.

Yeah, I agree that this makes sense, we just need to know how long it will take to land this and when it will be on-prem, and decide if we want to hold off on merging this until we know this!

mydea · 2024-04-26T11:55:28Z

So I updated this to send attachments in the same envelope! Makes everything easier.
With this change, I also removed the attachments option from captureFeedback() and sendFeedback(), instead leveraging this normally through the event hint. This means you have to do:

Sentry.captureFeedback({ message: 'xx' }, { attachments: [attachment1] });

Same as with all other capture methods.

bruno-garcia · 2024-04-30T14:21:51Z

The change on ingest to accept feedback and attachments on the same envelope was done through:

feat(replay): separate feedback from attachments in the same envelope relay#3403

This change has been live on SaaS and made it to self hosted: 24.4.2

https://github.com/getsentry/relay/releases/tag/24.4.2

ryan953 · 2024-04-30T14:31:11Z

packages/feedback/src/core/sendFeedback.ts

-    });
-
-    if (client.emit) {
-      client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) });


I think we still want to have this beforeSendFeedback hook. This should a) start to capture any buffered replay b) add a breadcrumb to the replay

Long term it'd be better to move into the Actor::onClick event, so we get more replay before they start typing, but that should be a followup because of the quota impact

We do still emit this, but in captureFeedback ;)

aliu3ntry · 2024-05-01T22:55:15Z

Assuming this PR sends feedback+attach in same envelope, let me know before you merge so I can turn on the ingest FF! I'ts only set in s4s right now.

Turned on: accepts same envelope AND separate envs
Turned off: only accepts separate envs
Or you can make a PR to do so yourself like so: https://github.com/getsentry/sentry-options-automator/pull/1234

The change on ingest to accept feedback and attachments on the same envelope was done through:

feat(replay): separate feedback from attachments in the same envelope relay#3403

This change has been live on SaaS and made it to self hosted: 24.4.2

https://github.com/getsentry/relay/releases/tag/24.4.2

fix PR

… user feedback events

mydea self-assigned this Apr 4, 2024

mydea force-pushed the fn/captureFeedback branch from dfab71c to abf402e Compare April 9, 2024 14:36

Lms24 mentioned this pull request Apr 16, 2024

test(browser-integration-test): Add tests to check trace lifetime in user feedback events #11626

Closed

mydea force-pushed the fn/captureFeedback branch from abf402e to c16b325 Compare April 19, 2024 12:10

mydea marked this pull request as ready for review April 19, 2024 12:59

mydea requested review from ryan953, c298lee, billyvg and Lms24 April 19, 2024 12:59

Lms24 approved these changes Apr 19, 2024

View reviewed changes

billyvg approved these changes Apr 19, 2024

View reviewed changes

ryan953 reviewed Apr 19, 2024

View reviewed changes

c298lee approved these changes Apr 19, 2024

View reviewed changes

ryan953 mentioned this pull request Apr 19, 2024

feat(feedback): Create async bundles and code to resolve helper integrations #11621

Merged

ryan953 reviewed Apr 19, 2024

View reviewed changes

mydea force-pushed the fn/captureFeedback branch from a9b30eb to 743ab6d Compare April 26, 2024 11:54

mydea force-pushed the fn/captureFeedback branch from 743ab6d to 56fe6e5 Compare April 30, 2024 08:25

ryan953 reviewed Apr 30, 2024

View reviewed changes

ryan953 approved these changes Apr 30, 2024

View reviewed changes

ryan953 mentioned this pull request Apr 30, 2024

Add attachments to user feedback getsentry/sentry#49946

Open

mydea added 5 commits May 3, 2024 09:20

feat(feedback): Add captureFeedback method

cd2bb05

fix PR

test(browser-integration-test): Add tests to check trace lifetime for…

7f0aba5

… user feedback events

fix tests etc.

4e71967

export everywhere...

672a029

fix tests

a43bc1b

mydea added 6 commits May 3, 2024 09:20

more tests

8abfca8

one more test

3dde93a

attachments in single envelope

ba576ae

skip tests correctly

5c79ebd

fix lint

5b82f1a

fix format...

f14d0c8

mydea force-pushed the fn/captureFeedback branch from 78903fd to f14d0c8 Compare May 3, 2024 07:21

mydea requested a review from a team as a code owner May 3, 2024 07:21

mydea merged commit 56197af into develop May 3, 2024
98 checks passed

mydea deleted the fn/captureFeedback branch May 3, 2024 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mydea commented Apr 4, 2024 •

edited by ryan953

github-actions bot commented Apr 4, 2024 •

edited

Lms24 commented Apr 16, 2024

Lms24 left a comment

billyvg left a comment

ryan953 Apr 19, 2024

mydea Apr 22, 2024

ryan953 Apr 22, 2024

mydea Apr 30, 2024

ryan953 Apr 19, 2024

cmanallen Apr 19, 2024

c298lee Apr 19, 2024

aliu3ntry Apr 19, 2024 •

edited

cmanallen Apr 19, 2024 •

edited

aliu3ntry Apr 19, 2024

c298lee Apr 19, 2024

mydea Apr 22, 2024

ryan953 Apr 22, 2024

mydea Apr 23, 2024

mydea commented Apr 26, 2024

bruno-garcia commented Apr 30, 2024

ryan953 Apr 30, 2024

ryan953 Apr 30, 2024

mydea Apr 30, 2024

aliu3ntry commented May 1, 2024

feat(feedback): Add captureFeedback method #11428

feat(feedback): Add captureFeedback method #11428

Conversation

mydea commented Apr 4, 2024 • edited by ryan953

github-actions bot commented Apr 4, 2024 • edited

size-limit report 📦

Lms24 commented Apr 16, 2024

Lms24 left a comment

Choose a reason for hiding this comment

billyvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliu3ntry Apr 19, 2024 • edited

Choose a reason for hiding this comment

cmanallen Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Apr 26, 2024

bruno-garcia commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliu3ntry commented May 1, 2024

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

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

mydea commented Apr 4, 2024 •

edited by ryan953

github-actions bot commented Apr 4, 2024 •

edited

aliu3ntry Apr 19, 2024 •

edited

cmanallen Apr 19, 2024 •

edited