chore: add service_name label earlier in the ingestion pipeline #13702
Conversation
| lbs = lb.Set(LabelServiceName, serviceName).Labels() | ||
| s.Labels = lbs.String() | ||
|
|
||
| // Remove the added label after it's added to the stream so it's not consumed by subsequent steps |
There was a problem hiding this comment.
I wonder if this is the right call here 🤔 It's consistent with the current behavior so far, but I think it might be a bit confusing to users. They will suddenly see service_name everywhere, so they might wonder why they couldn't configure their retention periods with it too, right? 🤔
There was a problem hiding this comment.
I agree that this is kind of confusing but I believe the tenant retentions bytes here are used for billing so I'm a bit cautious to change it.
Also we've not made anything worse 😅
There was a problem hiding this comment.
LGTM in general, though I am not sure whether this makes sense for UX: https://github.com/grafana/loki/pull/13702/files#r1696836742
(though I am also not sure if we'll cause issues by keeping service_name there either)
| expectedBytes: len("fizzbuzz"), | ||
| expectedLines: 1, | ||
| expectedBytesUsageTracker: map[string]float64{`{foo="bar2"}`: float64(len("fizzbuss"))}, | ||
| expectedLabels: labels.FromStrings("foo", "bar2", LabelServiceName, ServiceUnknown), |
There was a problem hiding this comment.
expectedLabels I can't find its usage in the test 🤔
There was a problem hiding this comment.
Good catch. I really thought I added it here but I must not have because one of the tests was wrong.
|
|
||
| lb := labels.NewBuilder(lbs) | ||
| lbs = lb.Set(LabelServiceName, serviceName).Labels() | ||
| s.Labels = lbs.String() |
There was a problem hiding this comment.
i think we also need to set it back req.Streams[i] = s
If
discover_service_nameis enabled, this pr injects theservice_nameas part of the request parsing so we can do interesting things with it earlier than we could before.