Skip to content
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

Refactor the labels builder to remove the StreamLabel list #14307

Closed
ravishankar15 opened this issue Sep 30, 2024 · 3 comments
Closed

Refactor the labels builder to remove the StreamLabel list #14307

ravishankar15 opened this issue Sep 30, 2024 · 3 comments

Comments

@ravishankar15
Copy link
Contributor

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
#13955 (comment)

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
Copy link
Contributor Author

ravishankar15 commented Sep 30, 2024

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

@salvacorts
Copy link
Contributor

salvacorts commented Oct 15, 2024

Sorry for the delayed response @ravishankar15, I was on PTO and just came back.

fmt.go#L394 where we fetch the label if it is in base and send the category as StreamLabel

You're right, I was wrong, we do use add[StreamLabel]. Really good catch! 👏

When we initialise the builder we can copy all the labels to add[StreamLabel] and use the add[StreamLabel] for all cases

I don't think this would simplify things substantially but we may make things a bit more difficult to reason about.
As you pointed out, base is a read-only slice with the original stream labels, we then use the add array to overwrite/enrich the original labels from base.

IMO we are good as we are now. Moreover provided this is part of the critical path of Loki, I would avoid making changes here that wouldn't solve any bug or improve the performance.

Wdyt?

@ravishankar15
Copy link
Contributor Author

Yeah @salvacorts I agree with your thought I will close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants