-
Notifications
You must be signed in to change notification settings - Fork 385
RI-7218 Add telemetry collection for Vector Search page #4811
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
RI-7218 Add telemetry collection for Vector Search page #4811
Conversation
Code Coverage - Frontend unit tests
Test suite run success4997 tests passing in 659 suites. Report generated by 🧪jest coverage report action from 264e320 |
1c148ec
to
523a81d
Compare
0b87e6d
to
f64da8b
Compare
39baa30
to
4e8a96b
Compare
f64da8b
to
4bcf5eb
Compare
- refactor the CardHeader component to work with "onQueryCopy" callback - rework the telemetry collection on "copy command" for the Workbench - collect telemetry when clicking on the "copy command" button in Vector Search re #RI-7218
… query" This reverts commit 523a81d.
- we can't make the QueryCardHeader component reuable at this point, it need to be reimagined from the ground up, so we decide to copy the current implementation and rework inline what we need to make the telemetry for the vector search re #RI-7218
…reen for a query result re #RI-7218
…lapse a query result re #RI-7218
…ry result re #RI-7218
…uery results re #RI-7218
4bcf5eb
to
910e4a7
Compare
const collectTelemetryQueryReRun = (query: string) => { | ||
sendEventTelemetry({ | ||
event: TelemetryEvent.SEARCH_COMMAND_RUN_AGAIN, | ||
eventData: { | ||
databaseId: instanceId, |
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.
this function already takes a parameter, add another one - instanceId and you can remove it's definition from the component body
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.
Done. The telemetry collection is exported as util, outside of the component.
* We're really sorry to do that at this point, but this family fo components should be refactored to be reusable. | ||
* At this point, it requires a lot of effort to make it reusable, so we decided to copy the component as is, and apply the necessary changes inline. |
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.
this adds to the tech debt. What are the changes the original component would require to make it reusable?
Diff checking both of them I currently see a telemetry param change and that's all, so what else is going to be changed?
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.
Yes, I agree. It was discussed with @pawelangelow to proceed this way for now. Check the Notes section in the description of the pull request for more information.
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.
Yeah, let's copy it for now. We either slow down to do it properly or introduce a tech debt - no "good" scenario :(
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.
Ok, I went for a simplified approach, and I think it worked:
const collectTelemetryQueryRun = () => { | ||
sendEventTelemetry({ | ||
event: TelemetryEvent.SEARCH_COMMAND_SUBMITTED, | ||
eventData: { | ||
databaseId: instanceId, | ||
commands: [query], |
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.
again, absolutely not a fan of declaring such functions inside the component, especially since they only require 1-2 params. This adds unnecessary unnecessary complexity - you open the component body and half the code is related to a telemetry callbacks
seeing how much space these telemetry functions take I'd even suggest extracting them in a separate telemetry.ts file or something like that
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.
Well, they won't disappear completely as the handlers should live here:
const handleClearResults = () => {
onAllQueriesDelete()
sendEventTelemetrySomething()
}
...
and the remaining 3
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.
Both of you are correct. We still have the base handlers as part of the component, because we need them.
But I managed to export the telemetry collection logic into a helper util, outside of the component.
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.
And obviously, all the logic can be external to the component by incorporating a hook :)
But since this is mostly personal preference, I am saying this just for completenes and nothing else :)
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.
Initially, I went for a hook, but then @KrumTy shared some concerns on the topic and advised going for a simpler helper function, and keeping it independent of the React lifecycle. I think it works for now.
… (workbench or vector search) re #RI-7218
…ommand for both workbecnh and vector search re #RI-7218
…pnd query for both workbecnh and vector search re #RI-7218
… for both workbecnh and vector search re #RI-7218
redisinsight/ui/src/components/query/context/view-mode.context.tsx
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/components/query/context/view-mode.context.tsx
Outdated
Show resolved
Hide resolved
ViewModeContextProviderProps | ||
> = ({ | ||
children, | ||
initialViewMode = ViewMode.Workbench, // Default to Workbench if not provided |
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.
explicit over default is better in this case imo
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.
I prefer to keep this default value, for backward compatibility and safety.
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.
this is a new provider, no backwards compatibility is needed as you are defining the interface atm
also, from what I've seen, you are setting it explicitly everywhere (as should be) already
const telemetryEvent = | ||
viewMode === ViewMode.Workbench | ||
? isOpen | ||
? TelemetryEvent.WORKBENCH_RESULTS_COLLAPSED | ||
: TelemetryEvent.WORKBENCH_RESULTS_EXPANDED | ||
: isOpen | ||
? TelemetryEvent.SEARCH_RESULTS_COLLAPSED | ||
: TelemetryEvent.SEARCH_RESULTS_EXPANDED | ||
|
||
sendEvent(telemetryEvent, query) |
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.
this starts to look ugly
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.
Yes, we all agree on that. In general, the component should be refactored so it can be reused properly on multiple places without having such tight business logic inside it. Unfortunately, it's not that simple, so that’s why we went for this quick and dirty fix with the conditional telemetry.
Regarding this piece of code, the best thing I can do is to split it over two conditional operators, and it will still be ugly. So after all, I rely on general refactoring to fix this.
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.
So basically, this is a context that shares single value across components, right?
Is there any reason not to just add this to Redux store?
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.
it's used in different views now. It would not make sense to dispatch redux actions to update the store as you switch between Search and Workbench tabs
This context solution is the least path of resistance to reusing the QueryCard component. The other being prop drilling, which @valkirilov said was too hard (4-5 levels deep).
The pre-context solution was to copy the whole component and it's test and adjust 3-4 telemetry lines.
…nto fe/feature/RI-7218_vector-search-telemetry-3
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.
LGTM
Description
Added telemetry collection events to track users' interaction with the Vector Search page:
PS: The diff may look huge, but actually, we have the same changes applied on multiple places + copy/paste of a big component.
How to test it
http://localhost:8080/{databaseId}/vector-search
SEARCH_COMMAND_SUBMITTED
event)SEARCH_COMMAND_COPIED
event)SEARCH_COMMAND_RUN_AGAIN
event)SEARCH_RESULTS_IN_FULL_SCREEN
event)SEARCH_RESULTS_COLLAPSED
event)SEARCH_RESULTS_EXPANDED
event)SEARCH_CLEAR_RESULT_CLICKED
event)SEARCH_CLEAR_ALL_RESULTS_CLICKED
event)SEARCH_CLEAR_EDITOR_CLICKED
event)Screen.Recording.2025-08-07.at.14.03.20.mov
Verify Telemetry events
We have the new events for the Vector Search page collected successfully.
Note: Telemetry is collected only if the user decides to opt-in to that.
You control it from Settings page -> Privacy section -> Usage Data toggle.
Notes: Since the introduced changes reflect the Workbench telemetry as well, I have checked that we still emit properly the following events:
WORKBENCH_RESULTS_COLLAPSED
WORKBENCH_RESULTS_EXPANDED
WORKBENCH_COMMAND_COPIED
WORKBENCH_CLEAR_RESULT_CLICKED