-
Notifications
You must be signed in to change notification settings - Fork 301
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
Telemetry: At Mentions #5558
Telemetry: At Mentions #5558
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
e10bd19
to
f853036
Compare
0265798
to
ec7b187
Compare
f853036
to
80a00eb
Compare
ec7b187
to
aa7f4a1
Compare
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.
Left a few clarifying comments :)
80a00eb
to
bdafa0f
Compare
b123bcf
to
7bf10dd
Compare
7bf10dd
to
b9204ba
Compare
4e1ed31
to
d86e4b6
Compare
b9204ba
to
1b2a1f8
Compare
d86e4b6
to
3b09ee8
Compare
1b2a1f8
to
1aeed23
Compare
661fc38
to
12283bc
Compare
1aeed23
to
0b95742
Compare
@akalia25 what do you want to do with those failing telemetry events in the other tests? Can we somehow just remove the event from the general checks as we can do more detailed testing now? Or do I have to go in and update them all? |
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.
Left a few comments regarding the collection of transcripts, and changes introduced to that. After those, I can approve :)
repoMetadata, | ||
repoIsPublic, | ||
traceId: span.spanContext().traceId, | ||
promptText: inputText, |
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.
For the collection of transcript data, we'd need to do the same isDotCom()
check that was happening before, is this happening somewhere else in the code now?
The reason for this is two-fold, first prevents the local PG instance from storing this data and increasing db load and second for additional security around this sensitive data.
promptText: inputText, | |
promptText: | |
isDotCom(authStatus) && | |
(await truncatePromptString(inputText, CHAT_INPUT_TOKEN_BUDGET)), | |
}, |
hmm I'd love to have those updated 😅 , those have been critical in us spot-checking events. Happy to assist and update them as well! |
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.
manually tested
Asked @jan Hartman before and he said the prompt text wasn't used. For now we're prioritizing making sure DAU count is correct. We can easily add back prompt text back later.
Before the
cody.at-mention
event fired as soon as the chat view was rendered leading to an overcount of DAUs. Duplicate mention events were also fired for other mention providers such as when typing a file name where each character would fire an event.Now events only fire when actually triggered. We also clean up and add additional public metadata that can help evaluate the actual effectiveness of different mention providers.
Fixes CODY-3405
Test plan
cd vscode & pnpm run test:e2e2 --grep "telemetry
.Changelog