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

Bump rustls version to 0.23 #459

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Bump rustls version to 0.23 #459

merged 5 commits into from
Jul 9, 2024

Conversation

bwoebi
Copy link
Contributor

@bwoebi bwoebi commented May 29, 2024

We were running into symbol issues with older rustls versions, see briansmith/ring#1808.

Note that the new rustls version requires nasm to be installed on windows, so we install it in CI.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2024

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.

Project coverage is 70.48%. Comparing base (b7a8566) to head (99483e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
+ Coverage   70.46%   70.48%   +0.01%     
==========================================
  Files         205      205              
  Lines       27936    27934       -2     
==========================================
+ Hits        19685    19688       +3     
+ Misses       8251     8246       -5     
Components Coverage Δ
crashtracker 16.72% <ø> (ø)
datadog-alloc 98.76% <ø> (ø)
data-pipeline 51.15% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.60% <65.00%> (+0.02%) ⬆️
ddcommon-ffi 75.31% <ø> (ø)
ddtelemetry 59.33% <ø> (ø)
ipc 84.66% <ø> (ø)
profiling 78.63% <ø> (ø)
profiling-ffi 58.19% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 36.14% <ø> (ø)
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 69.70% <ø> (ø)
trace-normalization 98.24% <ø> (ø)
trace-obfuscation 95.75% <ø> (ø)
trace-protobuf 77.16% <ø> (ø)
trace-utils 90.96% <0.00%> (+0.20%) ⬆️

@bwoebi bwoebi force-pushed the bob/bump-rustls branch 9 times, most recently from 2821933 to 182f14c Compare May 30, 2024 08:56
We were running into symbol issues with older rustls versions, see briansmith/ring#1808.

Note that the new rustls version requires nasm to be installed on windows, so we install it in CI.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@pr-commenter
Copy link

pr-commenter bot commented Jul 5, 2024

Benchmarks

Comparison

Parameters

Baseline Candidate
config baseline candidate
git_branch main bob/bump-rustls
git_commit_date 1720456555 1720457692
git_commit_sha b7a8566 99483e0
iterations 712.0 749.0
See matching parameters
Baseline Candidate
ci_job_date 1720457939 1720457939
ci_job_id 565483720 565483720
ci_pipeline_id 38601845 38601845
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.471µs; -3.456µs] or [-4.945%; -4.925%]
66.721µs 70.184µ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
749.0 candidate Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz 1720457939 565483720 38601845 99483e0 1720457692 bob/bump-rustls
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.654µs 66.721µs ± 0.026µs 66.724µs ± 0.017µs 66.736µs 66.763µs 66.769µs 66.795µs 0.11% -0.151 -0.033 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.716µs; 66.726µs] or [-0.008%; +0.008%] None None None

Warnings

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
712.0 baseline Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz 1720457939 565483720 38601845 b7a8566 1720456555 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 70.129µs 70.184µs ± 0.026µs 70.182µs ± 0.016µs 70.198µs 70.235µs 70.244µs 70.253µs 0.10% 0.340 -0.026 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 [70.179µs; 70.189µs] or [-0.007%; +0.007%] None None None

Warnings

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.
---------------------------------------------------------------------------

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi force-pushed the bob/bump-rustls branch 2 times, most recently from b430970 to 590c412 Compare July 8, 2024 14:30
…But aws-lc-sys is actually preferable.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi marked this pull request as ready for review July 8, 2024 15:36
@bwoebi bwoebi requested review from a team as code owners July 8, 2024 15:36
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.

🎉

@bwoebi bwoebi merged commit 25f5c42 into main Jul 9, 2024
32 checks passed
@bwoebi bwoebi deleted the bob/bump-rustls branch July 9, 2024 10:25
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.

3 participants