-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
reset the basemap when the labelsbuilder is reset #11771
Conversation
m[k] = v | ||
} | ||
} | ||
for k, v := range b.baseMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also moved this out of the guard because consecutive calls to IntoMap
turned out to also fail in addition to calls after Reset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure of this change. I don't know if the intention of the builder and baseMap
was that we only build a labelset once and then always return it, or if there are cases where we should allow different label sets to be passed to the same builder without calling Reset
. I think only the latter would require these lines to move.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that change, this test fails:
t.Run("it can copy the map several times", func(t *testing.T) {
b := NewBaseLabelsBuilder().ForLabels(lbs, lbs.Hash())
m := map[string]string{}
b.IntoMap(m)
require.Equal(t, map[string]string{
"namespace": "loki",
"job": "us-central1/loki",
"cluster": "us-central1",
"ToReplace": "text",
}, m)
m2 := map[string]string{}
b.IntoMap(m2)
require.Equal(t, map[string]string{
"namespace": "loki",
"job": "us-central1/loki",
"cluster": "us-central1",
"ToReplace": "text",
}, m2)
})
I'd sort of expect to be able to make any number of copies of the map as long as I'm aware of what I'm passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed on slack, I had my own code mixed up and was misunderstanding the use case and change here
This PR fixes an issue where the `label_format` stage sometimes doesn't populate labels when it should. The label `baz` exists on all log lines but is only populated on the first line. ![image](https://github.com/grafana/loki/assets/1413241/86585a94-937f-4e07-9657-b94699ce8586) The issue is this call to [IntoMap](https://github.com/grafana/loki/blob/06bb20914b027c96ac3af27ae36e91f35529524d/pkg/logql/log/fmt.go#L403). It tries to avoid copying the map multiple times [here](https://github.com/grafana/loki/blob/06bb20914b027c96ac3af27ae36e91f35529524d/pkg/logql/log/labels.go#L482) but the `baseMap` variable isn't reset when the labels builder is reset. As a result, calls to to `IntoMap` after the builder has been reset mean that `!b.hasDel() && !b.hasAdd() && !b.HasErr()` evaluates to `true` but `b.baseMap == nil` also evaluates to `true` so no items are copied. This PR makes it so that `b.baseMap` is reset along with the rest of the builder's state in `Reset`
This PR fixes an issue where the `label_format` stage sometimes doesn't populate labels when it should. The label `baz` exists on all log lines but is only populated on the first line. ![image](https://github.com/grafana/loki/assets/1413241/86585a94-937f-4e07-9657-b94699ce8586) The issue is this call to [IntoMap](https://github.com/grafana/loki/blob/06bb20914b027c96ac3af27ae36e91f35529524d/pkg/logql/log/fmt.go#L403). It tries to avoid copying the map multiple times [here](https://github.com/grafana/loki/blob/06bb20914b027c96ac3af27ae36e91f35529524d/pkg/logql/log/labels.go#L482) but the `baseMap` variable isn't reset when the labels builder is reset. As a result, calls to to `IntoMap` after the builder has been reset mean that `!b.hasDel() && !b.hasAdd() && !b.HasErr()` evaluates to `true` but `b.baseMap == nil` also evaluates to `true` so no items are copied. This PR makes it so that `b.baseMap` is reset along with the rest of the builder's state in `Reset`
This PR fixes an issue where the
label_format
stage sometimes doesn't populate labels when it should.The label
baz
exists on all log lines but is only populated on the first line.The issue is this call to IntoMap. It tries to avoid copying the map multiple times here but the
baseMap
variable isn't reset when the labels builder is reset. As a result, calls to toIntoMap
after the builder has been reset mean that!b.hasDel() && !b.hasAdd() && !b.HasErr()
evaluates totrue
butb.baseMap == nil
also evaluates totrue
so no items are copied.This PR makes it so that
b.baseMap
is reset along with the rest of the builder's state inReset