-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add global labels value regression tests #191
Conversation
A basic test to verify the converted bug in handling tags.
Refactor to allow running the same test logic with different inputs and expected results.
Add an e2e test for global labels values with multiple events. A batch of 2 events in input with different labels Values but same key should produce in output events with all different Values added to the same key.
d2abbb7
to
3f629f5
Compare
This PR adds 2 tests to catch a bug in Global Labels values marshalling/unmarshalling:
I expect test 1 to not fail in this branch, as the bug should have been fixed. What is puzzling me and I don't yet have an answer for is test 2 because:
I investigated a bit where this bug may be but haven't found it yet. |
We only retain the latest tags in a batch of events.
TestAggregateAndHarvest sort logic only used Metricset.Name. The latest test added to it s flappy with such a logic, as we are evaluating the same Metricset.Names in 2 different events with global labels with equal key and different values. Enhance the sorting logic to account for equal Metricset.Name and leverage labels (keys, Value and Values) in the lessFn.
When 2 events in a batch have the same service name but different tags we output 2 different sets of metrics, one for each tag set. This was not considered in the previous expected values.
aggregators/aggregator_test.go
Outdated
if akeys[i] != bkeys[i] { | ||
return akeys[i] < bkeys[i] | ||
} | ||
|
||
akey := akeys[i] | ||
if a.Labels[akey].Value != "" && a.Labels[akey].Value != b.Labels[akey].Value { | ||
return a.Labels[akey].Value < b.Labels[akey].Value | ||
} |
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.
For reviewers: these tests are not directly used with the current tests. I added them anyway to make this sorting function more robust and prevent unexpected failures in the future if we update the test cases.
This reverts commit 49d0906. The same test is covered by TestMarshalEventGlobalLabelsRace, which also test for a possible race condition.
aggregators/aggregator_test.go
Outdated
return a.Metricset.Name < b.Metricset.Name | ||
} | ||
|
||
// otherwise sort by sorted labels |
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.
nit: not sure if it ever can manifest, but probably worth checking that both a and b Labels slices have the same size, otherwise this sorting can panic if slices are not equal.
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 added a guard clause to prevent this, what do you think? 3b3a56b
(#191)
The previous implementation could panic if len(b) > len(a). Add a guard clause that limit iterating over label based on len(b). If len(a.Labels) > len(b.Labels), everything else equal we conclude that a > b.
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.
thanks for the test. One comment about the big sort func.
Leverage a string comparison as a sort function, as used in https://github.com/elastic/apm-data/blob/81d77648a3b17d4e52859110e233e40683fcdfe3/input/otlp/metrics_test.go#L1146-L1148 Adds an additional tag to the test case to ensure expected results.
Provide tests to ensure no regression will happen in handling global labels
Values
field.Closes #189