-
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
Svelte: Add basic telemetry to svelte implementation #62190
Conversation
} | ||
} | ||
|
||
export const SVELTE_LOGGER = new SvelteTelemetry() |
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.
Should this be part of the context rather than a global?
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 was thinking about this but then I couldn't come up with reasons why it shouldn't be a global singleton. There might be an argument that this could be better for mocking in tests but I can say that there are no big differences in vi.mock
vs context mocking, curious about what @fkling thinks
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.
We should really get our formatters all in sync 😄
78b4919
to
e07292f
Compare
To make sure we have time to test it on staging and with amplitude I'm going ahead and merging this, will be happy to change anything later next week if something shows up |
Closes #61635
Context
This PR moves the telemetry events out of the web package to the shared package. This allows using this telemetry service in the
web-sveltekit
package. Also, the wrapper over the original telemetry adds a specialisSveltePrototype: true
parameter to make a difference later between the original event and the svelte events.Second thing, this PR adds only a subset of the original event; if you see that some event is missing, please leave a note. Currently, this adds these events
For reviewers
Diff got a lot of files due to the telemetry service moving. Check the commit history for easier review.
Next steps
Note that this PR uses old telemetry V1 due to needs around migration amplitude dashboards to new v2 telemetry events. I have a follow up here and I aim to bring new telemetry in June release #62191
Test plan