Skip to content
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

Error telemetry in cloud #3516

Merged
merged 7 commits into from
Nov 29, 2023
Merged

Conversation

AdityaHegde
Copy link
Collaborator

@AdityaHegde AdityaHegde commented Nov 17, 2023

Checklist

  • Manual verification
  • Unit test coverage
  • E2E test coverage
  • Needs manual QA?

Summary

Issue addressed:

We do not have any feedback from cloud UI.

Details:

As a first step adding an event for error faced by the users in cloud.

Data is directly pulled from the kafka topic basically skipping the intake client. So the end store is the same as the dev pipeline.

Steps to Verify

@AdityaHegde AdityaHegde marked this pull request as draft November 17, 2023 11:52
@AdityaHegde AdityaHegde force-pushed the adityahegde/cloud-error-telemetry branch from 4fba9c2 to 7455526 Compare November 17, 2023 12:16
@AdityaHegde AdityaHegde self-assigned this Nov 17, 2023
@AdityaHegde AdityaHegde marked this pull request as ready for review November 17, 2023 12:18
@ericpgreen2
Copy link
Contributor

Can you add handling for client-side errors at runtime?

I think something like this:

window.onerror = (msg, url, line, col, error) => {
    // capture error & emit telemetry
    // show a full error page
};
window.onunhandledrejection = (event) => {
    // capture error & emit telemetry
    // show a full error page
};

@ericpgreen2
Copy link
Contributor

Can you add details in the PR description about where the telemetry gets sent?

@ericpgreen2
Copy link
Contributor

ericpgreen2 commented Nov 27, 2023

This is looking good! I haven't QA'd it though. Can add a screenshot of the various telemetry in action in the downstream viz tool? Rill Cloud or Rill Legacy? It'd be very cool if this telemetry were included as a dashboard in the internal-rill-cloud-metrics project.

Comment on lines 28 to 34
$: if ($page.params.project && $user.data?.user?.id) {
metricsService.loadCloudFields({
isDev: window.location.host.startsWith("localhost"),
projectId: $page.params.project,
userId: $user.data?.user?.id,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What about tracking the organizationId too?
  2. Can we load the userId from the AvatarButton.svelte component? Because the userID will be present if the user is logged in on the Home and Org pages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added organizationId. It might be better to load all the data at a single place.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Left a comment on metricsService.loadCloudFields(), and there's still a leftover console log, else looks good!

Looking forward to seeing the Rill dashboard!

@AdityaHegde AdityaHegde merged commit a5086e0 into main Nov 29, 2023
5 checks passed
@AdityaHegde AdityaHegde deleted the adityahegde/cloud-error-telemetry branch November 29, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants