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

DEBUG-2509 Add option to skip checks when building profiling #513

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

p-datadog
Copy link
Contributor

What does this PR do?

Adds -T option to build-profiling-ffi.sh script to skip post-build checks, which is frequently done during development.

Also adds a usage blurb which can be printed with -h option, and prints the usage blurb if the required positional parameter of destination directory is not provided instead of failing with a non-descriptive error.

Motivation

Current diagnostics are not indicating what is wrong when the positional argument is not supplied.

Additional Notes

None

How to test the change?

Run ./build-profiling-ffi.sh -h or ./build-profiling-ffi.sh -T.

Adds -T option to build-profiling-ffi.sh script to skip
post-build checks, which is frequently done during development.

Also adds a usage blurb which can be printed with -h
option, and prints the usage blurb if the required
positional parameter of destination directory is not provided
instead of failing with a non-descriptive error.
@p-datadog p-datadog requested a review from a team as a code owner June 27, 2024 20:00
@pr-commenter
Copy link

pr-commenter bot commented Jun 27, 2024

Benchmarks

Comparison

Parameters

Baseline Candidate
config baseline candidate
git_branch main optional-tests
git_commit_date 1719511185 1719518421
git_commit_sha 15aa48d 8d95f19
iterations 714.0 708.0
See matching parameters
Baseline Candidate
ci_job_date 1719518657 1719518657
ci_job_id 555647953 555647953
ci_pipeline_id 37785626 37785626
cpu_model Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz

Summary

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

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
708.0 candidate Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz 1719518657 555647953 37785626 8d95f19 1719518421 optional-tests
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.480µs 70.555µs ± 0.031µs 70.557µs ± 0.023µs 70.577µs 70.608µs 70.626µs 70.629µs 0.10% 0.066 -0.576 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.549µs; 70.562µ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.
---------------------------------------------------------------------------

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
714.0 baseline Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz 1719518657 555647953 37785626 15aa48d 1719511185 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 69.968µs 70.030µs ± 0.034µs 70.029µs ± 0.028µs 70.055µs 70.086µs 70.095µs 70.103µs 0.11% 0.075 -0.836 0.05% 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.023µs; 70.037µ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 Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.37%. Comparing base (15aa48d) to head (8d95f19).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #513   +/-   ##
=======================================
  Coverage   70.37%   70.37%           
=======================================
  Files         204      204           
  Lines       27824    27824           
=======================================
  Hits        19581    19581           
  Misses       8243     8243           
Components Coverage Δ
crashtracker 16.70% <ø> (ø)
datadog-alloc 98.76% <ø> (ø)
data-pipeline 51.15% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.10% <ø> (ø)
ddcommon-ffi 74.15% <ø> (ø)
ddtelemetry 59.37% <ø> (ø)
ipc 84.66% <ø> (ø)
profiling 78.63% <ø> (ø)
profiling-ffi 58.19% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 36.19% <ø> (ø)
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 69.70% <ø> (ø)
trace-normalization 97.79% <ø> (ø)
trace-obfuscation 95.75% <ø> (ø)
trace-protobuf 77.16% <ø> (ø)
trace-utils 91.21% <ø> (ø)

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I know @r1viollet has been working to get rid of this script, but this seems a useful + harmless UX improvement to have until we can kill it off :)

@p-datadog p-datadog merged commit 44cedac into main Jun 28, 2024
32 checks passed
@p-datadog p-datadog deleted the optional-tests branch June 28, 2024 16:16
@p-datadog p-datadog changed the title Add option to skip checks when building profiling DEBUG-2509 Add option to skip checks when building profiling Jul 1, 2024
duncanpharvey pushed a commit that referenced this pull request Jul 1, 2024
Adds -T option to build-profiling-ffi.sh script to skip
post-build checks, which is frequently done during development.

Also adds a usage blurb which can be printed with -h
option, and prints the usage blurb if the required
positional parameter of destination directory is not provided
instead of failing with a non-descriptive error.

Co-authored-by: Oleg Pudeyev <code@olegp.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants