-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
More key feature instrumentation #2811
Conversation
These definitions seem complex and and thus hard to keep stable over time. Having the definition fluctuate will in turn lead to more complexity in the long-term. To dig a bit: |
Thank you so much for taking the time to think deeply about this and the push back. There's more context on #2809, however the goal of these more complex metrics is to have a metric that represents a strong signal that the user is actually using the feature in a way that generates value. For example, merely opening the Regardless, just documenting the above for posterity, as we've agreed to on Slack, we'll continue this conversation in Jan. |
2679c90
to
ca38afa
Compare
Ready for another look @macobo, as discussed, it doesn't make sense right now to over-complicate things with an arbitrary timeout period. I'd appreciate if you can help me test the E2E behavior. |
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.
Added some possible issues, LGTM otherwise (feel free to merge once resolved)
}, | ||
listeners: { | ||
reportAnnotationViewed: async ({ annotations }: { annotations: AnnotationType[] | null }, breakpoint) => { | ||
if (!annotations) { |
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.
Did not test this, but I'd assume breakpoint(500) would need to be first right - otherwise we'd still report it as viewed.
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.
Q: Is it worth having code for the cancel case at all - KISS.
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.
Don't think the breakpoint should go first, because the annotations
won't change while the breakpoint is happening. If the variable is null, why wait, it will never lead to something getting reported anyways.
I clarified the comment a bit. Basically if the var is not set, we can't report usage.
Changes
Introduces more custom front-end events as planned on #2809.
In addition:
eventUsageLogic
to centralize custom front-end events reporting.Checklist