v2t: add v2 telemetry to the client/shared folder#62586
Conversation
I haven't checked the existing dashboards with these events, but why wouldn't it work for us if we had an event with the same Feature name but differences in action type in eventProperties as dynamic arguments? I think it still would be possible to make a difference in amplitude/looker dashboards. I thought that part of the reason why we migrated to the new telemetry mechanism was partially about having strict/fixed event/feature names to minimize the surface of telemetry names. Same suggestions about |
Safe metadata only accepts numeric fields and known keys, i.e. a similar problem will arise where we still need to have a known set of all the possible values here (either translated to int enum, or for inclusion in keys), which is by design (we shouldn't be able to record things we don't know about) |
|
I was able to sort of "solve" this in these two commits in this PR by requiring all features be (Thanks @bobheadxi for giving me the tip of upgrading the version of the |
|
@dadlerj I'm 80% sure that won't work - locally I see: I'm going to poke around this this afternoon as well and see if I can come up with something for ya |
|
Hah, interesting, because I don't get that locally (and I DO get successful events logged). I wonder if the I haven't dug into CI issues yet though and I wouldn't be surprised if my local configuration isn't enforcing the rules correctly due to weird package versioning issues that I'm drowning in. |
|
This problem will only fail the typechecker, but by nature of TS you don't need to fulfill the typechecker for things to run :) |
|
Hm, quickly ran into a dead end with Even when the interface declares a generic parameter: |
|
I think I understand the problem a bit better now - https://github.com/GoogleChromeLabs/comlink has a type - interface { foo(): bar }
+ interface { foo(): Promise<Remote<bar>> }i.e. function call In this process it strips out most typechecking except the most basic primitives, i.e. - interface { foo<T extends string>(): bar }
+ interface { foo<string>(): Promise<Remote<bar>> }You can see this in action if you remove - extHostAPI: Promise<Remote<FlatExtensionHostAPI>>
+ extHostAPI: Promise<FlatExtensionHostAPI>You get tons of compilation errors about In Cody agent protocol, we receive over events over the wire as well, and there we forcibly cast types into |
|
@bobheadxi can you check the commit I just pushed? Obviously lots of issues, but curious if this is roughly the structure of what you were proposing? |
There was a problem hiding this comment.
Here is the example I'm thinking of: https://sourcegraph.com/github.com/sourcegraph/cody/-/blob/agent/src/agent.ts?L684-695; this works because 'feature' fulfills the type requirements of KnownString (sorry that my other comments on this were a bit confusing)
| action.telemetryProps.feature as KnownString<Feature>, | |
| action.telemetryProps.feature as 'feature', |
Note the docstring I put above in particular that explains the usage:
// 👷 HACK: We have no control over what gets sent over JSON RPC,
// so we depend on client implementations to give type guidance
// to ensure that we don't accidentally share arbitrary,
// potentially sensitive string values. In this RPC handler,
// when passing the provided event to the TelemetryRecorder
// implementation, we forcibly cast all the inputs below
// (feature, action, parameters) into known types (strings
// 'feature', 'action', 'key') so that the recorder will accept
// it. DO NOT do this elsewhere!I think we can adapt that docstring to this callsite as well, based on my comment in https://github.com/sourcegraph/sourcegraph/pull/62586#issuecomment-2111022872 (in other words, we can't enforce complex types over the wire)
There was a problem hiding this comment.
Thank you! This is much more clear now.
Up above you wrote:
in Cody agent, we still retain the typechecking at the callsite, but we can't do that here because of the use of Remote<>.
By this do you mean that it's actually impossible to do here for some reason? Or could I actually enforce that the action.telemetryProps.feature value is a KnownString? It seems like it would do roughly the same thing here as what you describe happens with the agent—the "callers" would then be enforcing the requirement here as well?
There was a problem hiding this comment.
No need to reply to this, the words being used here are hard to parse. I'll work on a next draft and get your take on that.
|
@vovakulikov @bobheadxi this PR is finally ready for review. I have it working locally and it passes CI! Let me know if you have any questions or comments, I'd love to get this in (and thus getting the pre-Svelte web codebase to ~100% v2 telemetry coverage) as soon as I can. |
bobheadxi
left a comment
There was a problem hiding this comment.
I'm not 100% an expert here, but if CI type checks pass, everything here overall LGTM! Unfortunate that there is so much stuff under /legacy-extensions that still need to be maintained, but thank you for powering through these 💪
There was a problem hiding this comment.
We should document the expected format here (maybe copied over from the telemetry recorder docstrings)
There was a problem hiding this comment.
| * @deprecated use recordEvent instead | |
| * @deprecated use getTelemetryRecorder().recordEvent instead |
| this.props.telemetryService.log(action.id) | ||
| if (action.telemetryProps) { | ||
| this.props.telemetryRecorder.recordEvent( | ||
| // 👷 HACK: We have no control over what gets sent over Comlink/ |
There was a problem hiding this comment.
Is this a Comlink issue though? It looks like that in this place we don't run anything in web-worker, no?
ActionItem is a react component and it's used under WebHoverOverlay, or are you referring that we get
actions through extensions API which is web-workers
There was a problem hiding this comment.
It's a problem with how the comlink library thing in use here manipulates types - see https://github.com/sourcegraph/sourcegraph/pull/62586#issuecomment-2111022872
For example, in ActionContribution, we make use of a type called TemplateExpression. In a properly type-checked scenario you'll run into type errors if you try to cast a string into TemplateExpression directly - you have to use parse(...) from the package. But, all callsites registerContributions(...) provide strings directly for fields that are TemplateExpression - because Comlink turns the type signature into a string.
Declared type:
Post-conversion to Raw from Comlink:
Similarly, the type hack we use to try and force callsites to use a static string for feature/action gets removed by the Comlink
Codeintel v1 telemetry/event logging was broken in https://github.com/sourcegraph/sourcegraph/pull/62586 due to the lack of parens around a ternary operator. This simply fixes that issue. ## Test plan CI ## Changelog Co-authored-by: Dan Adler <5589410+dadlerj@users.noreply.github.com>
This is a tough one, given how much abstraction there is here...
V2 telemetry is built on a foundation of using known/safe strings only for event logging, while the /shared folder is almost exclusively filled with extensible abstractions like "action items", "commands", and "extensions" that can be created by any code intelligence client (web, browser extensions).
I've run into this concretely in two places so far, but both of these still need to be updated before I can mark this as ready for review and mergeable:
First, in ActionItem.tsx, I am currently simply logging every event as
blob.action/executed, with the action ID as a metadata parameter (whereas v1 telemetry logged the action ID as the event name itself. This really doesn't work for us—it means that things as critical as 'go to definition' and 'find references' are all logged as the abstractblob.action/executed... I need to figure out how to enforce theKnownStringtype on the action ID (or perhaps to add a new action property called eventFeature or similar that could have that constraint), which will require updates to all clients, unfortunately...Second, in commands.ts, I got lazy, and didn't log anything just marked it as TBD. I'm not really sure what an example of a command is yet (given all the abstraction here), but I assume enforcing the same
KnownStringrequirements will be necessary here.Test plan
sg start