Skip to content

Report telemetry on actions taken in the UI#1953

Merged
robertbrignull merged 15 commits intomainfrom
robertbrignull/telemetry_ui
Jan 17, 2023
Merged

Report telemetry on actions taken in the UI#1953
robertbrignull merged 15 commits intomainfrom
robertbrignull/telemetry_ui

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

When the user makes certain actions in the UI, output telemetry for it. This is done by sending a message from the webview back to the extension backend, which can then output telemetry in the normal way. We introduce a new telemetry event name of ui-interaction.

A large source of telemetry is from react state changes. If the data shown in the UI changes then there must have been a react state change, so we can use useState calls as an indicator of an event we want to emit telemetry for. Not all state changes should emit telemetry events, but only the ones that are the direct result of a user action. To make it easier, I've added a useStateWithTelemetry method that wraps useState and allows emitting telemetry whenever the value changes.

Another source of useful telemetry is when the user clicks a link. To cover these we have to identify the links and add onClick handlers to them.

I've manually tried to click every button and perform every action I can in the MRVA results view, and I think this now sends telemetry events for all actions.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some small suggestions, but in general it looks good. I clicked around the MRVA UI and agree with you that this should cover all events.

Comment thread extensions/ql-vscode/src/view/common/Telemetry.ts Outdated
Comment thread extensions/ql-vscode/src/view/common/FileCodeSnippet/CodeSnippetMessage.tsx Outdated
Comment thread extensions/ql-vscode/src/view/variant-analysis/VariantAnalysis.tsx Outdated
Comment thread extensions/ql-vscode/src/remote-queries/variant-analysis-view.ts
Comment thread extensions/ql-vscode/src/view/common/Telemetry.ts Outdated
@robertbrignull
Copy link
Copy Markdown
Contributor Author

@koesie10, @charisk, thank you both for the review. I believe I have now addressed all comments so this is ready for a re-review.

It was mentioned earlier today maybe we don't have to send an extension message and we could emit telemetry events from within the webview. I don't know if that's possible but I'm happy to explore it in a followup PR. It won't change the bulk of this work as sending the extension messages isn't the hard bit.

Comment thread extensions/ql-vscode/src/telemetry.ts Outdated
Comment thread extensions/ql-vscode/src/view/common/telemetry.ts
Comment thread extensions/ql-vscode/src/view/variant-analysis/RepoRow.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/telemetry.ts
Comment thread extensions/ql-vscode/src/view/variant-analysis/VariantAnalysis.tsx
Comment thread extensions/ql-vscode/src/view/common/telemetry.ts Outdated
Comment thread extensions/ql-vscode/src/view/common/telemetry.ts
@aeisenberg
Copy link
Copy Markdown
Contributor

aeisenberg commented Jan 16, 2023

I just saw this comment:

It was mentioned earlier today maybe we don't have to send an extension message and we could emit telemetry events from within the webview. I don't know if that's possible but I'm happy to explore it in a followup PR. It won't change the bulk of this work as sending the extension messages isn't the hard bit.

It would generally be possible to do this, but we'd need to change the CSP when we construct the webview. In the past, we've been careful to ensure that the webview only communicates with the extension. Maybe it's something worth reconsidering. However, I think you'd need to use a different library to send the telemetry events. This would mean that the structure of webview events would be different from extension events. This wouldn't be terrible, but would require some extra work in application insights.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

I just saw this comment:

It was mentioned earlier today maybe we don't have to send an extension message and we could emit telemetry events from within the webview. I don't know if that's possible but I'm happy to explore it in a followup PR. It won't change the bulk of this work as sending the extension messages isn't the hard bit.

It would generally be possible to do this, but we'd need to change the CSP when we construct the webview. In the past, we've been careful to ensure that the webview only communicates with the extension. Maybe it's something worth reconsidering. However, I think you'd need to use a different library to send the telemetry events. This would mean that the structure of webview events would be different from extension events. This wouldn't be terrible, but would require some extra work in application insights.

Thanks for explaining. That's really helpful! Sounds to me like maybe too much effort and downsides of having two implementations, unless sending the extension messages turns out to be a big problem. Right now I don't think the volume of extension messages or the implementation of the message is too bad.

@aeisenberg
Copy link
Copy Markdown
Contributor

Yes, and another benefit of using a single mechanism to send telemetry is that the package we are using right now batches multiple telemetry events into a single request.

@robertbrignull robertbrignull merged commit 611f6e3 into main Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants