v2t: add v2 telemetry to the client/shared folder

[dadlerj]

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 abstract blob.action / executed... I need to figure out how to enforce the KnownString type 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 KnownString requirements will be necessary here.

Test plan

1. sg start
2. visit page
3. check if events appear in event_logs table locally

[vovakulikov]

@dadlerj

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 abstract blob.action / executed

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 commands.ts, let me know if it wouldn't work I recently did the same thing for svelte prototype on old telemetry rails (by same I mean I used the same events from react app but made difference for svelte by enforcing svelte parameter in even properties, in amplitude at lease there is a way to parse JSON event bodies and select one property and filter datasets by this)

[bobheadxi]

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?

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)

[dadlerj]

I was able to sort of "solve" this in these two commits in this PR by requiring all features be KnownString types.

(Thanks @bobheadxi for giving me the tip of upgrading the version of the @sourcegraph/telemetry library to make this work!)

[bobheadxi]

@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

[dadlerj]

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.

[bobheadxi]

This problem will only fail the typechecker, but by nature of TS you don't need to fulfill the typechecker for things to run :)

[bobheadxi]

Hm, quickly ran into a dead end with extHostAPI: Promise<Remote<FlatExtensionHostAPI>> which seems to make generics not work, i.e. T is always unknown and Feature extends string is always just string, and when you provide a type parameter:

Even when the interface declares a generic parameter:

[bobheadxi]

I think I understand the problem a bit better now - https://github.com/GoogleChromeLabs/comlink has a type Remote that turns an interface into an "over the wire" interface, i.e. for communication between web workers, making changes like:

- interface { foo(): bar }
+ interface { foo(): Promise<Remote<bar>> }

i.e. function call foo() is now async and over the wire

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 Remote wrapping:

- extHostAPI: Promise<Remote<FlatExtensionHostAPI>>
+ extHostAPI: Promise<FlatExtensionHostAPI>

You get tons of compilation errors about Expression etc types where callsites omit the typically required adapters to turn a string into Expression. This kind of makes sense because over the wire it is impossible to enforce types, but makes for an unfortunate situation here.

In Cody agent protocol, we receive over events over the wire as well, and there we forcibly cast types into KnowString<'known'>, i.e. we pretend the string is a const known string. We can do that here as well, but we will completely lose the typechecking; in Cody agent, we still retain the typechecking at the callsite, but we can't do that here because of the use of Remote<>.

[dadlerj]

@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?

[bobheadxi]

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)

Suggested change

	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 #62586 (comment) (in other words, we can't enforce complex types over the wire)

[dadlerj]

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?

[dadlerj]

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.