Skip to content
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

ekump/adding-test-agent-to-trace-utils #516

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

ekump
Copy link
Contributor

@ekump ekump commented Jul 2, 2024

What does this PR do?

Introduces the test agent to libdatadog to be used for integration tests in trace-utils.

The test agent runs inside a docker container that is spun up/down within a given test via testcontainers-rs.

The DatadogTestAgent struct has been created as a wrapper around the testcontainers implementation. It also provides some helper functions to make it easier to write tests that leverage the test agent.

A single integration test has been added that performs a snapshot test via the test-agent for a trace.

Motivation

There is imminent work around trace_utils planned that could potentially modify trace payloads. These tests should provide some confidence for that work.

Additional Notes

The new tests naturally require docker. The README has been updated with instructions on how to skip these tests locally if you're not running docker. Alternatively, if people feel strongly we can invert this and default to not running these tests by default locally via a cargo feature.

The test will run on github CI only on Ubuntu for the time being.

How to test the change?

cargo nextest run

@pr-commenter
Copy link

pr-commenter bot commented Jul 2, 2024

Benchmarks

Comparison

Parameters

Baseline Candidate
config baseline candidate
git_branch main ekump/add-test-agent-to-trace-utils-2
git_commit_date 1720520743 1720531028
git_commit_sha 25f5c42 92c3fdd
iterations 748.0 787.0
See matching parameters
Baseline Candidate
ci_job_date 1720531380 1720531380
ci_job_id 566586950 566586950
ci_pipeline_id 38702639 38702639
cpu_model Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz

Summary

Found 1 performance improvements and 0 performance regressions! Performance is the same for 0 metrics, 0 unstable metrics.

scenario Δ mean execution_time candidate mean execution_time baseline mean execution_time
scenario:sql/obfuscate_sql_string better
[-3.248µs; -3.226µs] or [-4.867%; -4.835%]
63.493µs 66.730µs

Candidate

Candidate benchmark details

Group 1

iterations config cpu_model ci_job_date ci_job_id ci_pipeline_id git_commit_sha git_commit_date git_branch
787.0 candidate Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz 1720531380 566586950 38702639 92c3fdd 1720531028 ekump/add-test-agent-to-trace-utils-2
scenario metric min mean ± sd median ± mad p75 p95 p99 max peak_to_median_ratio skewness kurtosis cv sem runs sample_size
sql/obfuscate_sql_string execution_time 63.390µs 63.493µs ± 0.046µs 63.489µs ± 0.032µs 63.527µs 63.572µs 63.582µs 63.590µs 0.16% -0.003 -0.637 0.07% 0.005µs 1 100
scenario metric 95% CI mean Shapiro-Wilk pvalue Ljung-Box pvalue (lag=1) Dip test pvalue
sql/obfuscate_sql_string execution_time [63.484µs; 63.502µs] or [-0.014%; +0.014%] None None None

Warnings

Scenario: sql/obfuscate_sql_string, Metric: execution_time

Measurements are autocorrelated.

Autocorrelation is present for lags 1..10.

The measurements are not independent, thus confidence intervals
may be less precise.
---------------------------------------------------------------------------
Scenario: sql/obfuscate_sql_string, Metric: execution_time

Sample size is 100, which is lower than 105.

The minimal sample size in case of normal distribution to achieve significance
level of 0.05 for difference of means with effect size Cohen's d = 0.5 must be at
least 105.

The conclusions from confidence intervals may be invalid.
---------------------------------------------------------------------------

Baseline

Baseline benchmark details

Group 1

iterations config cpu_model ci_job_date ci_job_id ci_pipeline_id git_commit_sha git_commit_date git_branch
748.0 baseline Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz 1720531380 566586950 38702639 25f5c42 1720520743 main
scenario metric min mean ± sd median ± mad p75 p95 p99 max peak_to_median_ratio skewness kurtosis cv sem runs sample_size
sql/obfuscate_sql_string execution_time 66.639µs 66.730µs ± 0.030µs 66.736µs ± 0.018µs 66.751µs 66.774µs 66.784µs 66.786µs 0.08% -0.635 0.503 0.04% 0.003µs 1 100
scenario metric 95% CI mean Shapiro-Wilk pvalue Ljung-Box pvalue (lag=1) Dip test pvalue
sql/obfuscate_sql_string execution_time [66.724µs; 66.736µs] or [-0.009%; +0.009%] None None None

Warnings

Scenario: sql/obfuscate_sql_string, Metric: execution_time

Measurements are autocorrelated.

Autocorrelation is present for lags 1..10.

The measurements are not independent, thus confidence intervals
may be less precise.
---------------------------------------------------------------------------
Scenario: sql/obfuscate_sql_string, Metric: execution_time

Sample size is 100, which is lower than 105.

The minimal sample size in case of normal distribution to achieve significance
level of 0.05 for difference of means with effect size Cohen's d = 0.5 must be at
least 105.

The conclusions from confidence intervals may be invalid.
---------------------------------------------------------------------------

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 96.80851% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.30%. Comparing base (25f5c42) to head (92c3fdd).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
+ Coverage   70.21%   70.30%   +0.08%     
==========================================
  Files         205      206       +1     
  Lines       27712    27806      +94     
==========================================
+ Hits        19458    19549      +91     
- Misses       8254     8257       +3     
Components Coverage Δ
crashtracker 16.86% <ø> (ø)
datadog-alloc 98.73% <ø> (ø)
data-pipeline 51.15% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.68% <ø> (ø)
ddcommon-ffi 75.31% <ø> (ø)
ddtelemetry 59.02% <ø> (ø)
ipc 84.13% <ø> (ø)
profiling 78.68% <ø> (ø)
profiling-ffi 58.26% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 35.69% <ø> (ø)
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 70.93% <ø> (ø)
trace-normalization 98.24% <ø> (ø)
trace-obfuscation 95.75% <ø> (ø)
trace-protobuf 77.16% <ø> (ø)
trace-utils 90.68% <96.80%> (+0.25%) ⬆️

@ekump ekump force-pushed the ekump/add-test-agent-to-trace-utils-2 branch from fb44a50 to d4f05fa Compare July 2, 2024 14:54
@ekump ekump force-pushed the ekump/add-test-agent-to-trace-utils-2 branch 5 times, most recently from 464a373 to f56fcee Compare July 2, 2024 18:23
@ekump ekump changed the title WIP adding initial integration tests ekump/adding-test-agent-to-trace-utils Jul 2, 2024
@ekump ekump force-pushed the ekump/add-test-agent-to-trace-utils-2 branch from f22875c to de0358b Compare July 2, 2024 18:54
@ekump ekump marked this pull request as ready for review July 2, 2024 18:55
@ekump ekump requested review from a team as code owners July 2, 2024 18:55
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I, for one, welcome our new docker running overlords...

@ekump ekump force-pushed the ekump/add-test-agent-to-trace-utils-2 branch from de0358b to 3a33805 Compare July 8, 2024 13:03
Copy link
Contributor

@VianneyRuhlmann VianneyRuhlmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm impressed by how smooth it is to run test with docker within cargo 🤩

README.md Outdated Show resolved Hide resolved
trace-utils/Cargo.toml Outdated Show resolved Hide resolved

async fn get_base_uri_string(&self) -> String {
let container_host = self.container.get_host().await.unwrap().to_string();
let container_port = self.container.get_host_port_ipv4(8126).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use TEST_AGENT_PORT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch.

@ekump ekump force-pushed the ekump/add-test-agent-to-trace-utils-2 branch 2 times, most recently from 7459183 to f12fd00 Compare July 8, 2024 17:31
ekump and others added 7 commits July 9, 2024 09:13
ephemeral docker container. Also, add initial baseline snapshot test.
Only run tracing integration tests on ubuntu for now. Filter tracing
integraiton tests out of regular tests and run specifically in its own
step.
Co-authored-by: vianney <vianney.ruhlmann@datadoghq.com>
@ekump ekump force-pushed the ekump/add-test-agent-to-trace-utils-2 branch from 87734be to 92c3fdd Compare July 9, 2024 13:18
@ekump ekump merged commit d57d6d4 into main Jul 9, 2024
32 checks passed
@ekump ekump deleted the ekump/add-test-agent-to-trace-utils-2 branch July 9, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants