Refactor metrics controller tests to fix flaky behaviour#4406
Conversation
Moved testing that is polls through Eventually() tests into the metric assert function - since all metric data is eventually consistent. Also centralizes edge cases around Distribution metrics. Closes agones-dev#4391
|
Build Succeeded 🥳 Build Id: b4e464c4-e76b-4aeb-b988-cdbf418cb7be The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
| hasDistribution := false | ||
| for _, e := range expected { | ||
| expectedValuesAsMap[serialize(e.labels)] = e | ||
| if _, ok := e.val.(*metricdata.Distribution); ok { | ||
| hasDistribution = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| var wantedMetric *metricdata.Metric | ||
| for _, m := range exporter.metrics { | ||
| if m.Descriptor.Name == metricName { | ||
| wantedMetric = m | ||
| } | ||
| // For Distribution metrics, handle the synchronization and reset pattern | ||
| if hasDistribution { | ||
| // Wait until the metric is available | ||
| assert.Eventually(t, func() bool { | ||
| f() | ||
| exporter := &metricExporter{} | ||
| c.lock.Lock() | ||
| reader.ReadAndExport(exporter) | ||
| c.lock.Unlock() | ||
| for _, m := range exporter.metrics { | ||
| if m.Descriptor.Name == metricName { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| }, 10*time.Second, 100*time.Millisecond) |
There was a problem hiding this comment.
I am not sure why this fixes things, but basically just copying the structure of other working tests. OpenCensus can be weird.
There was a problem hiding this comment.
I wonder if it was not because of a race condition, and that the lock / unlock around reader.ReadAndExport you added would be the patch ?🤔 There must be other things as well
There was a problem hiding this comment.
I tried with just an eventually added to the final assert... and things did not go as planned. In theory it should have worked - but this seemed good enough 🤷🏻 it tested what we wanted to test.
The locking is to avoid data races collecting and then exporting metrics, since opencensus is not thread safe! It happened very rarely when I was running this lots.
Side note - didn't know assert.EventuallyWithT was a thing - I could refactor some tests to use it. Very handy.
|
Oh yeah - I also ran all the tests in |
| hasDistribution := false | ||
| for _, e := range expected { | ||
| expectedValuesAsMap[serialize(e.labels)] = e | ||
| if _, ok := e.val.(*metricdata.Distribution); ok { | ||
| hasDistribution = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| var wantedMetric *metricdata.Metric | ||
| for _, m := range exporter.metrics { | ||
| if m.Descriptor.Name == metricName { | ||
| wantedMetric = m | ||
| } | ||
| // For Distribution metrics, handle the synchronization and reset pattern | ||
| if hasDistribution { | ||
| // Wait until the metric is available | ||
| assert.Eventually(t, func() bool { | ||
| f() | ||
| exporter := &metricExporter{} | ||
| c.lock.Lock() | ||
| reader.ReadAndExport(exporter) | ||
| c.lock.Unlock() | ||
| for _, m := range exporter.metrics { | ||
| if m.Descriptor.Name == metricName { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| }, 10*time.Second, 100*time.Millisecond) |
There was a problem hiding this comment.
I wonder if it was not because of a race condition, and that the lock / unlock around reader.ReadAndExport you added would be the patch ?🤔 There must be other things as well
…4406) Moved testing that is polls through Eventually() tests into the metric assert function - since all metric data is eventually consistent. Also centralizes edge cases around Distribution metrics. Closes agones-dev#4391
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Moved testing that is polls through Eventually() tests into the metric assert function - since all metric data is eventually consistent.
Also centralizes edge cases around Distribution metrics.
Which issue(s) this PR fixes:
Closes #4391
Special notes for your reviewer:
N/A