-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(tracing): create spans for all click events #9002
Conversation
size-limit report 📦
|
@@ -26,7 +26,7 @@ const BUNDLE_VARIANTS = ['.js', '.min.js', '.debug.min.js']; | |||
export function makeBaseBundleConfig(options) { | |||
const { bundleType, entrypoints, jsVersion, licenseTitle, outputFileBase, packageSpecificConfig } = options; | |||
|
|||
const isEs5 = jsVersion.toLowerCase() === 'es5'; | |||
const isEs5 = jsVersion && jsVersion.toLowerCase() === 'es5'; |
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 had some compliation errors without this 🤷
I can delete if we want, but i don't see any harm in keeping it.
84b4d22
to
fd5c666
Compare
This reverts commit fd5c666.
inflightInteractionTransaction.startChild({ | ||
description: htmlTreeAsString(target), | ||
op, | ||
startTimestamp: timestampInSeconds(), | ||
endTimestamp: timestampInSeconds() + 0.001, | ||
}); |
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.
Ideally we can get the timestamps from performance entries, but for now i'm just marking this in the event handler.
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.
do we also need to set a finish reason here?
9ef6a03
to
d8c53c3
Compare
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 it lgtm, maybe we can get @AbhiPrasad to take a quick look over the code before me merge?
inflightInteractionTransaction.startChild({ | ||
description: htmlTreeAsString(target), | ||
op, | ||
startTimestamp: timestampInSeconds(), | ||
endTimestamp: timestampInSeconds() + 0.001, | ||
}); |
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.
do we also need to set a finish reason here?
Some work towards #getsentry/team-sdks/issues/32
This ensures interaction transactions are created with the appropriate OP for click, mousedown, mouseup, and contextmenu events.
Click - Click events work the same as before, an idle transaction is created in the event handler, and the performance entry for the click will be added as a span if available. The op is
ui.action.left.click
Mousedown/mouseup - Mousedown will start a regular transaction, a mousedown span is added, the transaction will end when a mouseup occurs and a mouseup span will be added. The op is
ui.action.left.mousedown
.Unfortunately the performance entry needs more work to be added here. The performance entry handler fires after the three event handlers, 'mouseup', 'moundown` and 'click' occurs, which is after the mousedown transaction finishes.
Contextmenu (aka right click) - context menu work the same as clicks, an idle transaction is created, the performance entry is added as a span if available. The op is
ui.action.right.contextmenu
. Handling mouseup/mousedown for right click is a bit more involved, so i didn't handle it here.This seems to work well with the demo app i've created, but i'm going to leave this as draft until I test it a bit more. Early feedback is encouraged! If this is good behaviour for transaction creation, then getting better spans and grouping interaction is a good next step!