search

sourcegraph / about

Sourcegraph blog, feature announcements, and website (about.sourcegraph.com)
https://about.sourcegraph.com
90 stars 168 forks source link

Remove v1 telemetry and fully replace with v2 telemetry #6971

Closed dadlerj closed 3 weeks ago

dadlerj commented 1 month ago

Follow up to https://github.com/sourcegraph/about/pull/6967

This PR:

Tested locally and using CI

dadlerj commented 1 month ago

Vova, feel free to tell me if you're not a good reviewer for this one!

netlify[bot] commented 1 month ago

Deploy Preview for sourcegraph ready!

Name Link
Latest commit 9f3f87a387a47d796b89f1fc01d7876c07e50efb
Latest deploy log https://app.netlify.com/sites/sourcegraph/deploys/666285621de9db0008018a98
Deploy Preview https://deploy-preview-6971--sourcegraph.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dadlerj commented 1 month ago

@vovakulikov using enums for event names gets a bit less clean, as we now split event "names" into features and actions, rather than a single string.

In my opinion, the more important thing is to avoid using variables as event names, which we (to some degree), get from the KnownString type in the telemetry package. You can see I still occasionally use string interpolation for sub-features, but I'd love to avoid that (and I'll take your recommendation above!).

As long as the string itself is searchable, I don't particularly care if it's in an enum or just a hardcoded string. What's your reaction to that?

dadlerj commented 3 weeks ago

@gitstart-sourcegraph that would be great, can you take a stab at it? Let me know if any significant changes are required and I can dig in.