-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
chore(feedback): Analytics for error rendering feedback item #78978
Conversation
if (issueResult.isError) { | ||
trackAnalytics('feedback.feedback-item-render-error', {organization, feedbackId}); | ||
} |
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.
Is there no sentry error when this happens? We should not need to capture error analytics :)
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.
hmm I don't see anything in Issues....
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.
Here's a replay of clicking into a feedback that errors: https://sentry.sentry.io/replays/abd88bfa16114ef5b682f88d203b223a/?n_detail_row=341&project=11276&query=user.email%3Acatherine.lee%40sentry.io&referrer=%2Freplays%2F%3AreplaySlug%2F&statsPeriod=90d&t_main=network&yAxis=count%28%29&t=140 no Sentry error tho
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 seems like there was no error because of the issueData?
down below: it's possible that the data doesn't exist and we handle it without throwing. Not an error, but also it wasn't a good user experience until now.
Tracking how often are people asking for feedbacks that don't exist seems reasonable, maybe we should tweak the analytics name
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.
How about feedback.feedback-item-missing-data
? or no-data
feedback.feedback-item-not-found
?
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 think I did something similar for feedback trace data, I also called the event an "error"..
Is 404 the only likely cause of isError
here?
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.
Nit: can split these if conditions to separate useEffect's
@@ -49,7 +49,7 @@ export default function useFetchFeedbackData({feedbackId}: Props) { | |||
// Until that is fixed, we're going to run `markAsRead` after the issue is | |||
// initially fetched in order to speedup initial fetch and avoid race conditions. | |||
useEffect(() => { | |||
if (issueResult.isFetched && !issueData?.hasSeen) { | |||
if (issueResult.isFetched && issueData && !issueData.hasSeen) { |
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.
nice catch!
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.
Adds analytics for error when rendering feedback item. Also fixes loading screen when the feedback does not exist
Adds analytics for error when rendering feedback item. Also fixes loading screen when the feedback does not exist