perf: Improve Detected labels API #12816
Conversation
shantanualsi
changed the title
| return false | ||
| _, err := strconv.ParseFloat(v, 64) | ||
| if err != nil { | ||
| _, err = uuid.Parse(v) |
There was a problem hiding this comment.
much cleaner than the regex pattern
| } | ||
| detectedLabels = append(detectedLabels, &logproto.DetectedLabel{ | ||
| Label: k, | ||
| Cardinality: v.Estimate(), |
There was a problem hiding this comment.
yeah, good call here, we hopefully don't care about this but if there's only 1 split we'll need the estimate here
There was a problem hiding this comment.
sorry didn't get the issue.. even if there's a 1 split, the estimate will be populated here right?
| var result []*logproto.DetectedLabel | ||
|
|
||
| for _, dl := range resp.Response.DetectedLabels { | ||
| if dl.Cardinality > 2 && dl.Cardinality < 50 { |
There was a problem hiding this comment.
I still get confused by our middlewares, so just to make sure, this is happening after we MergeLabels right? I wouldn't want to omit results based on partial cardinality of a subquery.
There was a problem hiding this comment.
yes confirmed again this is executed after MergeLabels. The line res, err := rt.Do(ctx, req) is before everything else in this function
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)docs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR