Order dependent bug - #46673
Conversation
3db3654 to
db5edb3
Compare
|
\assign @adisuissa |
There was a problem hiding this comment.
Pull request overview
Fixes flaky, order-dependent failures in the stats access logger tests by ensuring tests that trigger the same ENVOY_LOG_PERIODIC_MISC callsite run in separate test binaries, avoiding cross-test rate-limit suppression within a single process.
Changes:
- Extracted the shared
StatsAccessLoggerTestfixture (and supporting mocks) into a new header for reuse. - Split formatting/logging-related tests into a new
stats_formatting_testtarget to avoid log rate-limit coupling with other tests. - Updated
stats_testto keep the remaining tests and re-enable log assertions where rate-limiting is no longer shared.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/extensions/access_loggers/stats/stats_test.h | New shared test fixture/mocks header used by multiple test binaries. |
| test/extensions/access_loggers/stats/stats_test.cc | Removes formatting-focused tests and keeps core stats access logger tests; uses shared fixture header. |
| test/extensions/access_loggers/stats/stats_formatting_test.cc | New test binary containing formatting/logging-related cases that previously caused order-dependent flake. |
| test/extensions/access_loggers/stats/BUILD | Adds new stats_formatting_test target and includes shared header in both test targets. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::vector<std::unique_ptr<Stats::StatNameDynamicStorage>> name_storages_; | ||
| std::shared_ptr<Stats::MockScope> scope_; | ||
| std::unique_ptr<Stats::StatNameDynamicStorage> scope_name_storage_; | ||
| std::shared_ptr<StatsAccessLog> logger_; |
|
Assigning @antoniovleonti for a first-pass |
|
High level suggestion. Instead of reading the logs, the code paths in stats.cc being tested should increment a counter and the tests should check that counter. |
804a2ee to
55fd311
Compare
antoniovleonti
left a comment
antoniovleonti
left a comment
There was a problem hiding this comment.
Thanks, I like this approach much better, just a few suggestions.
| } | ||
|
|
||
| if (std::isnan(value) || value < 0.0 || | ||
| value > static_cast<double>(std::numeric_limits<uint64_t>::max())) { |
There was a problem hiding this comment.
I suspect there's a floating point precision bug here, can you add a test where value = 18446744073709551616.0?
I think static_cast<double>(uint64_max) gets rounded up to 18446744073709551616.0 (uint64_max+1). Meaning value = 18446744073709551616.0 is treated as in-range... Then converting this back to uint64 is UB.
You can fix this by replacing this check with:
value >= 18446744073709551616.0
(emphasis on the greater-than-or-equal)
| return std::nullopt; | ||
| } | ||
| } else if (computed_value.has_null_value()) { | ||
| return std::nullopt; |
There was a problem hiding this comment.
You should add a new test that exercises this branch
| value *= Stats::Histogram::PercentScale; | ||
| } | ||
|
|
||
| if (std::isnan(value) || value < 0.0 || |
There was a problem hiding this comment.
You should also add tests for value = NaN and value < 0.0
| return Stats::ScopeProviderSingleton::getScope(context, modified_config); | ||
| }()), | ||
|
|
||
| stats_({ALL_STATS_ACCESS_LOGGER_STATS( |
There was a problem hiding this comment.
Stats have some additional overhead (specifically memory).
Adding stats is typically ok if the stats are useful for an operator to look at during runtime. However, if they are only needed for a test, then they might not be needed, and depending on the cardinality (the number of stats that will be created during a normal execution of an Envoy), it may be better to avoid.
One suggestion is to look at the how many of these objects (and their stats) are created, and decide accordingly.
There was a problem hiding this comment.
After discussing this in-person with @antoniovleonti , he suggested an alternative that I believe is better than the approach I originally had in mind. I'll let him comment on that.
There was a problem hiding this comment.
Thanks for the feedback regarding the memory overhead. I'll wait for @antoniovleonti's suggestions on the alternative approach before proceeding with further changes.
|
RE: Adi's comment At first my suggestion was that getFormatValue should just return a StatusOr instead of optional. That way, we can distinguish between different error modes by the status. The problem with this is, this can't bubble up to logger_->log() because when it encounters a null / invalid value it just skips instead of early returning. So you would need to add a public interface for getFormatValue, so we can unit test it directly. Which is not the end of the world, but ... I think we can fix these tests without changing the source code. The tests already have a The existing tests already do this: Just remove the log expectation: - EXPECT_LOG_CONTAINS("error", "Stats access logger formatted a string that isn't a number: hello",
- { logger_->log(formatter_context_, stream_info_); });
+ logger_->log(formatter_context_, stream_info_);And you can just remove the stats stuff you added (oops, sorry). |
Signed-off-by: Wenwei Zhao <wenweizhao@google.com>
327d53d to
069ed65
Compare
I've removed the added stats counter from the source code and updated the unit tests to drop the |
Commit Message: Fix flaky order-dependent stats_test failures via physical test file separation
Additional Description: The tests
NonNumberValueFormattedandGaugeNonNumberValueFormattedboth trigger identical error logs emitted viaENVOY_LOG_PERIODIC_MISCinstats.cc. Because this macro rate-limits logging to prevent log spam, running these tests consecutively in the same target process led to one suppressing the other, causing flaky test failures under randomized execution ordering.Risk Level: low
Testing: NA
Docs Changes: NA
Release Notes: NA
Platform Specific Features: NA