-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
metrics(feedback): Track source emitted by SDK fxs #70810
Conversation
Unsure about naming - could be client_type, client_source_type.. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #70810 +/- ##
==========================================
+ Coverage 69.05% 77.92% +8.86%
==========================================
Files 6573 6573
Lines 292834 292783 -51
Branches 50542 50531 -11
==========================================
+ Hits 202220 228140 +25920
+ Misses 83892 58395 -25497
+ Partials 6722 6248 -474
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
tags={"referrer": source.value}, | ||
tags={ | ||
"referrer": source.value, | ||
"client_source": event["contexts"]["feedback"].get("source"), |
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 event["contexts"]["feedback"].get("source")
different from source.value
?
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.
Yeah -- the feedback context is SDK-defined and passed from there. Think source is required, but using .get in case of typescript problems. This is either "widget" or "api" (see code refs in description)
The source
variable is a FeedbackCreationSource enum, which is passed by Sentry backend when calling create_feedback_issue
. So it's a bit of a naming conflict
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.
ahhh gotcha, makes sense
This is a useful property we're not tracking in metrics -- right now we assume all NEW_FEEDBACK_ENVELOPE's are from the widget, but they could be sent by the public api too.
Code refs:
source types
widget onSubmit
sendFeedback (api)