-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
size-limit report 📦
|
packages/browser/src/userfeedback.ts
Outdated
tunnel: client.getOptions().tunnel, | ||
}); | ||
|
||
void transport.send(envelope); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
packages/browser/src/userfeedback.ts
Outdated
tunnel: client.getOptions().tunnel, | ||
}); | ||
|
||
void transport.send(envelope); |
There was a problem hiding this comment.
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.
packages/browser/src/userfeedback.ts
Outdated
tunnel: client.getOptions().tunnel, | ||
}); | ||
|
||
void transport.send(envelope); |
There was a problem hiding this comment.
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.
Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
This reverts commit e295944.
How about adding envelope promises here and returning it? Without it it's not possible to handle api error (at least to my knowledge). |
@fliespl Would you mind elaborating? |
Currently if I want to build custom user feedback and send request using:
there is no way to find out if it was successfully sent or not. I was hoping for this function to reurn fetch/XHR promise that allows to do specific operation depending on success/failure. |
@fliespl There are technical reasons for us not to expose whether an event (which user feedback is) was successfully sent or not. The SDK follows a fire-and-forget policy for all telemetry. The storing of events happens asynchronously and could be slow. The ingestion endpoint itself doesn't know whether a request was successfully sent so it will always return a success response with the exception of rate limiting. We will likely not change anything about this. |
@lforst understood. I was hoping I could match: showReportDialog which shows error in case feedback is not sent, but that's because it operates differently I guess. |
This adds an API to the Sentry Python SDK that captures user feedback via envelope. This is implemented very similiarly to how it is done for the JavaScript SDK, see getsentry/sentry-javascript#7729. Fixes getsentryGH-1064
This adds an API to the Sentry Python SDK that captures user feedback via envelope. This is implemented very similiarly to how it is done for the JavaScript SDK, see getsentry/sentry-javascript#7729. Fixes getsentryGH-1064
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (
yarn lint
) & (yarn test
).closes: Add an API for User Feedback #6252
Next