grafana / loki

Like Prometheus, but for logs.
https://grafana.com/loki
GNU Affero General Public License v3.0
23.51k stars 3.4k forks source link

Refactor the labels builder to remove the StreamLabel list #14307

Open ravishankar15 opened 2 days ago

ravishankar15 commented 2 days ago

Is your feature request related to a problem? Please describe. The current labels builder has two list base and add[StreamLabel] which is confusing and the add[StreamLabel] might not be needed since all the values are available in base

Describe the solution you'd like https://github.com/grafana/loki/pull/13955#discussion_r1735713847

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

ravishankar15 commented 2 days ago

On checking the stream label part. I think its been used in rename case https://github.com/grafana/loki/blob/main/pkg/logql/log/fmt.go#L394 where we fetch the label if it is in base and send the category as StreamLabel. On taking a deeper look,

I feel the idea behind the StreamLabel is,

  1. The base will hold the initial set of labels when initialising the builder
  2. The categories like StreamLabel, StructuredMetadataLabel and ParsedLabel will hold the values corresponding to that category and StreamLabel will hold values from base like the changed value from pipeline or other places using the builder. Like if using the builder i get a label A and update its value. When reading the label A it will be fetched from base and when updating it will be stored in the add[StreamLabel]. In nut shell the base is used as a readonly list of labels and add[StreamLabel] is used to hold the current value.

This idea can be validated in other methods of the builder,

  1. The Reset does not actually reset the base labels only resets the add lists https://github.com/grafana/loki/blob/main/pkg/logql/log/labels.go#L207
  2. Explicit check to see if the base has a label BaseHas https://github.com/grafana/loki/blob/main/pkg/logql/log/labels.go#L285C25-L285C32
  3. The Set method does not update the base labels https://github.com/grafana/loki/blob/main/pkg/logql/log/labels.go#L350

Based on my hypothesis, Instead of merging the base labels whenever we deal with stream labels. When we initialise the builder we can copy all the labels to add[StreamLabel] and use the add[StreamLabel] for all cases. For specific cases where we need the initial data we can use the base as of now we are not doing that, As we can see in https://github.com/grafana/loki/blob/main/pkg/logql/log/labels.go#L443 we populate the base if its a StreamLabel.

Correct me if my understanding is wrong.

cc: @salvacorts