Conversation
| import type { TelemetryEventInput, TelemetryExporter } from '@sourcegraph/telemetry' | ||
|
|
||
| // todo(dan) update with new recordeventresult type? | ||
| import type { logEventResult } from '../../graphql-operations' |
There was a problem hiding this comment.
Dumb question... the recordEvents mutation always returns alwaysNil. Do I have to go generate a new GQL typing for this specific mutation return value? Or is there some default alwaysNilResult type I can use instead?
I'm not really sure if this question makes sense but hopefully it does 😅
There was a problem hiding this comment.
I don't think it really matters, since we don't make use of anything from the results anyway
| return isFirefox() ? 'firefox-extension' : 'chrome-extension' | ||
| } | ||
|
|
||
| export function getTelemetryClientName(): string { |
There was a problem hiding this comment.
I distinguish between "native integrations" (i.e., those with a .integration suffix) and "browser extensions" (.browserExtension). I could be convinced to change this if you have any suggestions @bobheadxi
There was a problem hiding this comment.
I don't see anything wrong with this, if they're meaningfully different clients that you want to be able to distinguish in telemetry 😁 Naming scheme aligning with feature/action is also nice
|
@vovakulikov I tagged you on this because you have v2t context. But if anyone else (I see Felix and Valery were suggested by GitHub) is better equipped to review the bext code, feel free to tag them in instead! |
bobheadxi
left a comment
There was a problem hiding this comment.
Not sure I have the required expertise to approve the setup/teardown/etc but overall LGTM 😁
vovakulikov
left a comment
There was a problem hiding this comment.
No major requests, left a few comments.
|
|
||
| useEffect( | ||
| () => () => { | ||
| if (telemetryRecorder !== noOpTelemetryRecorder) { |
There was a problem hiding this comment.
@dadlerj why do we need this conditional unsubscribe? it looks like this check will work when we previously had noOpTelemetryRecorder and now we have a real one (so we're turning on telemetry I guess) and we call unsubscribe on the real telemetryRecorder
There was a problem hiding this comment.
This was giving me an error since noOpTelemetryRecorder doesn't have an unsubscribe() method. The other solution here would be to add a no-op unsubscribe to noOpTelemetryRecorder, I suppose. Would that be better here?
There was a problem hiding this comment.
Oh I missed that this is an unsub function for useEffect, in this case it's ok to have this, but in general yeah, I think if noOpTelemetryRecorder implements the same interface and common recorder it should have unsub method as well to reduce checks like this here.
There was a problem hiding this comment.
In this specific case, this is a little bit hard to do, since I'm importing a pre-existing factory class (NoOpTelemetryRecorderProvider) that generates the noOpTelemetryRecorder. So in order to add an unsubscribe method, I'd have to override both of those classes. I'm going to go ahead and merge this as-is and accept the awkwardness here, if that's alright, just to avoid adding a bunch of extra boilerplate... Feel free to DM me if you think it would be beneficial to do that!
…grations
This PR adds v2t telemetry infrastructure to the Sourcegraph browser extensions and native integrations code base.
Test plan
Changelog