Conversation
There was a problem hiding this comment.
Pull request overview
Updates functional test helpers to reduce golden fixture churn and improve metric-batch selection diagnostics when expected metric counts don’t line up exactly.
Changes:
- Strip trace schema URLs before comparing traces (and before writing updated golden traces).
- Update
SelectMetricSetto prefer exact count matches, otherwise fall back to the most “complete” metrics batch and return whether an exact match was found. - Propagate the
exactMatchsignal to selected functional tests for better failure logging.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| functional_tests/istio/istio_test.go | Uses updated metrics selection signature; clears trace schema URLs prior to trace comparisons. |
| functional_tests/internal/common.go | Adds ClearTraceSchemaURLs; changes metrics batch selection API to return fallback + exact-match bool. |
| functional_tests/functional/functional_test.go | Clears trace schema URLs in multiple language trace tests; uses new metrics selection return values and logs when not exact-match. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for h := len(metricSink.AllMetrics()) - 1; h >= 0; h-- { | ||
| m := metricSink.AllMetrics()[h] | ||
| foundTargetMetric := false | ||
|
|
||
| OUTER: | ||
| for i := 0; i < m.ResourceMetrics().Len(); i++ { | ||
| for j := 0; j < m.ResourceMetrics().At(i).ScopeMetrics().Len(); j++ { | ||
| for k := 0; k < m.ResourceMetrics().At(i).ScopeMetrics().At(j).Metrics().Len(); k++ { | ||
| metric := m.ResourceMetrics().At(i).ScopeMetrics().At(j).Metrics().At(k) | ||
| if metric.Name() == targetMetric { | ||
| foundTargetMetric = true | ||
| break OUTER | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !foundTargetMetric { | ||
| if !containsMetric(m, targetMetric) { | ||
| continue | ||
| } | ||
|
|
||
| if ignoreLen || (m.ResourceMetrics().Len() == expected.ResourceMetrics().Len() && m.MetricCount() == expected.MetricCount()) { | ||
| selectedMetrics = &m | ||
| t.Logf("Found target metric '%s' in payload with %d total metrics", targetMetric, m.MetricCount()) | ||
| if m.ResourceMetrics().Len() == expected.ResourceMetrics().Len() && m.MetricCount() == expected.MetricCount() { | ||
| exactMatch = &m | ||
| break | ||
| } | ||
| if m.MetricCount() > fallbackCount { | ||
| fallback = &m | ||
| fallbackCount = m.MetricCount() |
There was a problem hiding this comment.
SelectMetricSet stores &m where m is the loop variable (m := metricSink.AllMetrics()[h]). In Go this results in returning a pointer to the same variable reused across iterations, so exactMatch/fallback can end up pointing at the wrong payload. Create a per-iteration copy before taking the address (or store the index and take the address of a stable slice element).
| require.Eventuallyf(t, func() bool { | ||
| selectedMetrics = SelectMetricSet(t, expected, targetMetric, metricSink, ignoreLen) | ||
| selectedMetrics, exactMatch = SelectMetricSet(t, expected, targetMetric, metricSink) | ||
| return selectedMetrics != nil | ||
| }, timeout, interval, "Failed to find target metric %s within timeout period of %v", targetMetric, timeout) |
There was a problem hiding this comment.
SelectMetricSetWithTimeout stops polling as soon as any payload containing the target metric exists (return selectedMetrics != nil). Since SelectMetricSet now returns a non-nil fallback even when counts don’t match, this can return early with an incomplete batch and miss a later exact-match / more complete batch that arrives before the timeout. Consider continuing to retry until an exact match is found (tracking the best fallback seen so far) and only returning the fallback after the timeout expires.
pjanotti
left a comment
pjanotti
left a comment
There was a problem hiding this comment.
LGTM, with a very nit suggestion.
| var exactMatch *pmetric.Metrics | ||
| var fallback *pmetric.Metrics |
There was a problem hiding this comment.
Perhaps I'm being guided by the diff above in which exactMatch was a boolean, but felt that this nit makes more readable:
| var exactMatch *pmetric.Metrics | |
| var fallback *pmetric.Metrics | |
| var exactMatchMetrics *pmetric.Metrics | |
| var fallbackMetrics *pmetric.Metrics |
Description:
Ignore schema URL in trace comparisons across all functional tests - these bump with most upstream SDK release and adds more test fixture maintenance
SelectMetricSetfalls back to the most complete payload when no exact count match exists. Also returns a bool indicating whether an exact match was found, logged on failure for diagnostics.