-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ACIX-835] Update testify to v1.11.1 #40182
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
Conversation
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: b5f3d90 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | % cpu utilization | -6.23 | [-9.25, -3.21] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +0.86 | [+0.80, +0.92] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | +0.68 | [+0.56, +0.81] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | +0.44 | [+0.24, +0.63] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.17 | [+0.13, +0.20] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.06 | [-0.60, +0.73] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | +0.05 | [+0.00, +0.10] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | +0.04 | [-0.07, +0.15] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | +0.02 | [-0.11, +0.14] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.01, +0.01] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.00 | [-0.25, +0.25] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.02 | [-0.06, +0.02] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | -0.03 | [-0.61, +0.54] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | -0.05 | [-0.58, +0.49] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.07 | [-0.65, +0.51] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -0.12 | [-0.17, -0.08] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | -0.20 | [-0.37, -0.03] | 1 | Logs |
| ➖ | ddot_metrics_sum_delta | memory utilization | -0.81 | [-0.96, -0.65] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | -1.04 | [-1.10, -0.98] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -1.66 | [-4.42, +1.11] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -1.69 | [-1.89, -1.48] | 1 | Logs bounds checks dashboard |
| ➖ | docker_containers_memory | memory utilization | -4.98 | [-5.14, -4.82] | 1 | Logs |
| ✅ | docker_containers_cpu | % cpu utilization | -6.23 | [-9.25, -3.21] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
|
41aa326 to
46ddae6
Compare
louis-cqrl
left a comment
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.
LGTM for arun files
|
Omg they finally merged this fix! |
BaptisteFoy
left a comment
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.
lgtm for fleet
| require.Equal(t, value, actualEvents[evName], "event %s count mismatch", evName) | ||
| delete(actualEvents, evName) | ||
| } | ||
| require.EventuallyWithT(t, func(c *assert.CollectT) { |
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.
❓ question
Do we need these changes on .go files? They make the PR blurrier, are they a consequence of the bump?
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.
Yes it's a consequence
Basically the timing change in Eventually[WithT] made various tests flaky, I fixed most of them in a separate PR, but KMT tests were particularly tricky and had to be fixed by owning teams (who pushed to this PR)
I could technically split those commits in separate PRs too, but it seemed simpler to just keep the current state
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
devflow unqueued this merge request: It did not become mergeable within the expected time |
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
The expected merge time in
Gitlab pipeline didn't start... Please retry. Error: gitlab pipeline not started |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
### What does this PR do? Ensure that e2e tests are using testify v1.11.1 to benefit from stretchr/testify#1427. ### Motivation With the merge of #40182, I expected the elapsed time of containers e2e tests to be significantly reduced. Indeed, most of the tests are just waiting with an `EventuallyWithT` for an assertion against fake intake data to become true. And, most of the time, the assertion should be true at the first try. So, the 10+s of elapsed for the tests was coming from the `EventuallyWithT` initial try. And, according to [this CI page](https://app.datadoghq.com/ci/test/runs?query=test_level%3Atest%20%40test.service%3Adatadog-agent%20%40git.branch%3Amain%20%40test.suite%3A%22github.com%2FDataDog%2Fdatadog-agent%2Ftest%2Fnew-e2e%2Ftests%2Fcontainers%22%20%40test.name%3A%22TestEKSSuite%2FTestNginx%2Fmetric___network.http.response_time%7B%5Ekube_namespace%3Aworkload-nginx%24%7D%22&citest_explorer_sort=time%2Cdesc¤tTab=overview&eventStack=&fromUser=false&index=citest&mode=sliding&saved-view-id=2236559&start=1759918899379&end=1760523699379&paused=false), containers e2e tests are still lasting more than 10s despite the merge of #40182. ### Describe how you validated your changes I checked the *real* version of testify used by the e2e tests by: * starting them with: `dda inv -- -e new-e2e-tests.run --targets ./tests/containers --run TestEKSSuite` * looking for the test binary with: `pstree -p $(pgrep konsole)` * and finally checking the testify version with: `strings /proc/80184/exe | grep testify | grep v1.1` Before this PR, I had: ``` lenaic@lenaic-Precision-5570:~$ strings /proc/80184/exe | grep testify | grep v1.1 /home/lenaic/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertion_compare.go /home/lenaic/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertion_format.go /home/lenaic/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertion_order.go […] ``` With this PR, I now have: ``` lenaic@lenaic-Precision-5570:~$ strings /proc/90732/exe | grep testify | grep v1.1 /home/lenaic/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertion_compare.go /home/lenaic/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertion_format.go /home/lenaic/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertion_order.go […] ``` ### Additional Notes
What does this PR do?
Update testify to v1.11.1 https://github.com/stretchr/testify/releases/tag/v1.11.1
Motivation
Benefit from stretchr/testify#1427 (
assert: check early in Eventually, EventuallyWithT, and Never).Describe how you validated your changes
CI
Possible Drawbacks / Trade-offs
Additional Notes
The change means that some assertions now succeed earlier, which can make following assertions fail...
I had to fix various tests and increase some timeouts to avoid that.