chore(search): update logging of search durations#64269
Conversation
This updates and simplifies our logging as follows: - Prefer to log result types, fall back to pattern type. - Don't guess repo and file results types if not explicitly specified. - Allow "AND" and "OR" operators. Notes: - I left "New" and "legacy" logging in place. I am not sure how we use the target stores right now so I wanted to keep this as-is for now. However we should update this. - The previous and current logic translates `type:file` queries to pattern types. This makes sense from the perspective of how things are implemented in the backend, but it might be confusing for the consumer of the reporting. Test plan: updated unit test
| // categories. | ||
| if len(types) > 1 { | ||
| return | ||
| // Prefer the result type if it is a single type, otherwise use the pattern |
There was a problem hiding this comment.
This is mostly copy&pasted from below. New code from lines 140 on.
| return | ||
| clients.Logger.Warn("Could not log search latency", log.Error(err)) | ||
| } | ||
| if !query.IsPatternAtom(q) { |
There was a problem hiding this comment.
This means we never logged keyword searches with more than 1 term.
There was a problem hiding this comment.
Below it says // Not an atomic pattern, can't guarantee accurate reporting., I guess we don't think that's a concern anymore? I can't see a big issue with it, and it's critical to capture these cases for keyword search.
There was a problem hiding this comment.
I agree. I think we prefer completeness over accuracy here, especially now that keyword search is the default.
jtibshirani
left a comment
There was a problem hiding this comment.
This looks good to me. I like the simplification to not infer response types.
I am not familiar with our approach to new vs. legacy telemetry, and added the data team in case they have an opinion.
Also noting: we discussed offline about rethinking this reporting at some point (like why do we mix result and pattern types? and we shouldn't mix suggestions and API usage together with search bar usage)
| return | ||
| clients.Logger.Warn("Could not log search latency", log.Error(err)) | ||
| } | ||
| if !query.IsPatternAtom(q) { |
There was a problem hiding this comment.
Below it says // Not an atomic pattern, can't guarantee accurate reporting., I guess we don't think that's a concern anymore? I can't see a big issue with it, and it's critical to capture these cases for keyword search.
|
|
||
| // Search.latencies | ||
| events.Record(ctx, "search.latencies", telemetry.SafeAction(action), &telemetry.EventParameters{ | ||
| Metadata: telemetry.EventMetadata{ |
There was a problem hiding this comment.
It would be nice to capture more metadata here (like number of clauses). We have been wondering about how often people use multi-term searches, and therefore benefit from keyword search. Not necessary for this PR, just brainstorming!
There was a problem hiding this comment.
I agree. I will see if it is quick to add. If not I will file a follow up issue.
Relates to slack convo
The current logging is outdated. This PR is not a complete rewrite. It tries to keep semantics mostly as-is but it is more generous when it comes to what we log. (For example we didn't log
(AND foo bar)before)IMO this is a good compromise we can ship quickly and then take some time to figure out how to distinguish suggestions, searches, code-intel and whether it is a good idea to mix search types and result types.
Updates:
The updated test case gives a good overview of how we would log things in the future.
Kept as-is:
type:filequeries to pattern types. I guess this makes sense from the perspective of how things are implemented in the backend.Test plan:
Updated unit test