Support v0.5 trace endpoint #505

astuyve · 2024-06-25T20:30:12Z

What does this PR do?

Adds a few tiny lambda-related enums
Adds support for /v0.5/traces which are string-compressed trace chunks.

Motivation

I needed it

Additional Notes

I've tested this with python v0.5

How to test the change?

automated tests should be sufficient

codecov-commenter · 2024-06-25T20:36:41Z

Codecov Report

Attention: Patch coverage is 85.90308% with 32 lines in your changes missing coverage. Please review.

Project coverage is 70.35%. Comparing base (3ace4a5) to head (e650b86).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
+ Coverage   70.22%   70.35%   +0.12%     
==========================================
  Files         204      204              
  Lines       27581    27808     +227     
==========================================
+ Hits        19370    19565     +195     
- Misses       8211     8243      +32

Components	Coverage Δ
crashtracker	`16.70% <ø> (ø)`
datadog-alloc	`98.76% <ø> (ø)`
data-pipeline	`51.15% <ø> (ø)`
data-pipeline-ffi	`0.00% <ø> (ø)`
ddcommon	`85.97% <ø> (ø)`
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% <100.00%> (+0.07%)`	⬆️
trace-normalization	`97.79% <ø> (ø)`
trace-obfuscation	`95.75% <ø> (ø)`
trace-protobuf	`77.16% <ø> (ø)`
trace-utils	`91.21% <85.71%> (-0.61%)`	⬇️

trace-utils/src/trace_utils.rs

duncanista · 2024-06-25T21:13:44Z

trace-utils/src/trace_utils.rs

+    if span_size != 12 {
+        anyhow::bail!("Expected an array of exactly 12 elements in a span, got {span_size}");
+    }
+    //0 - service


So values come in order and I imagine that's why we are doing 12 reads here?

Yup, msgpack is self describing but as far as I know all fields are required here. Maybe Bjorn can confirm?

According to the docs, all are required: https://github.com/DataDog/datadog-agent/blob/579d60cbfc8635559338b19fa855bf59b6d10f00/pkg/trace/api/version.go#L110

Yes, the v0.5 is extremely rigid, and all fields are required.

trace-utils/src/trace_utils.rs

duncanista

LGTM – saw the e2e test, should we add individual tests?

morrisonlevi · 2024-06-25T22:20:56Z

trace-utils/src/trace_utils.rs

+                Vec<(
+                    u8,
+                    u8,
+                    u8,
+                    u64,
+                    u64,
+                    u64,
+                    i64,
+                    i64,
+                    i32,
+                    HashMap<u8, u8>,
+                    HashMap<u8, f64>,
+                    u8,
+                )>,


I'm not familiar with the domain here--is there a struct/proto or something I can look at to see what each slot of the tuple means? At a first glance, I am inclined to agree with clippy::type_complexity, although I realize this is just in a test.

great question, I wish I had one!
What I did find was this documentation and finally this test.

I used the same fields to create the test to confirm this all worked (as well as implementing it using the python tracer).

That doc comment is unfortunately the reference documentation.

bantonsson

Looks good. Only minor comments (and some nitpicks 😉)

trace-utils/src/trace_utils.rs

bantonsson · 2024-06-26T06:53:50Z

trace-utils/src/trace_utils.rs

+    if span_size != 12 {
+        anyhow::bail!("Expected an array of exactly 12 elements in a span, got {span_size}");
+    }
+    //0 - service


Yes, the v0.5 is extremely rigid, and all fields are required.

bantonsson · 2024-06-26T06:56:47Z

trace-utils/src/trace_utils.rs

@@ -40,6 +45,210 @@ pub async fn get_traces_from_request_body(body: Body) -> anyhow::Result<(usize,
    Ok((size, traces))
 }

+#[inline]
+fn get_v5_strings_dict(reader: &mut Reader<impl Buf>) -> anyhow::Result<Vec<String>> {


Could we name all the methods with _v05_ instead of _v5_ to use the same naming as _v04_?

bantonsson · 2024-06-26T07:19:24Z

trace-utils/src/trace_utils.rs

+            span.service = str_from_dict(dict, s)?;
+        },
+        val => anyhow::bail!("Error reading span service, value is not an integer and can't be looked up in dict: {val}")
+    };


This repeated code could be moved out to

fn get_v05_string(reader: &mut Reader<impl Buf>, dict: &[String], field_name: &str) -> anyhow::Result<String> { match read_value(reader)? { Value::Integer(s) => { str_from_dict(dict, s) }, val => anyhow::bail!("Error reading {field_name}, value is not an integer and can't be looked up in dict: {val}") } }

bantonsson · 2024-06-26T07:21:39Z

trace-utils/src/trace_utils.rs

+                Vec<(
+                    u8,
+                    u8,
+                    u8,
+                    u64,
+                    u64,
+                    u64,
+                    i64,
+                    i64,
+                    i32,
+                    HashMap<u8, u8>,
+                    HashMap<u8, f64>,
+                    u8,
+                )>,


That doc comment is unfortunately the reference documentation.

trace-utils/src/trace_utils.rs

…g log to figure out json string encoding stuff

bantonsson · 2024-06-27T08:26:56Z

trace-utils/src/trace_utils.rs

+    }
+}
+
+pub async fn v5_get_traces_from_request_body(


Almost all v5 to v05 😉

oops, fixed!

pr-commenter · 2024-06-27T13:24:47Z

Benchmarks

Comparison

Parameters

	Baseline	Candidate
config	baseline	candidate
git_commit_date	1719481384	1719494434
git_commit_sha	`3ace4a5`	`e650b86`
iterations	716.0	711.0

See matching parameters

	Baseline	Candidate
ci_job_date	1719494674	1719494674
ci_job_id	554759369	554759369
ci_pipeline_id	37686644	37686644
cpu_model	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
git_branch	aj/bottlecap-mini-agent	aj/bottlecap-mini-agent

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
711.0	candidate	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz	1719494674	554759369	37686644	`e650b86`	1719494434	aj/bottlecap-mini-agent

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.269µs	70.356µs ± 0.037µs	70.355µs ± 0.025µs	70.380µs	70.421µs	70.436µs	70.443µs	0.13%	0.021	-0.130	0.05%	0.004µ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.349µs; 70.364µs] or [-0.010%; +0.010%]	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
716.0	baseline	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz	1719494674	554759369	37686644	`3ace4a5`	1719481384	aj/bottlecap-mini-agent

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.667µs	69.847µs ± 0.101µs	69.827µs ± 0.046µs	69.879µs	70.054µs	70.178µs	70.230µs	0.58%	1.330	2.340	0.14%	0.010µ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	[69.827µs; 69.867µs] or [-0.028%; +0.028%]	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.
---------------------------------------------------------------------------

* feat: add Lambda * feat: env verifier * feat: set origin tag correctly * debug: what is v5 sending us * revert * feat: v0.5 trace decompression support * fix: remove comment and unwraps * feat: remove erroneous GCP comment from my copypasta * fix: complex type, license for rmpv * feat: Rename methods to use v05. Add v05 string method. Temp debugging log to figure out json string encoding stuff * wip: more debugging * fix: use into_str instead of to_string to avoid string escaping * feat: clean up match * feat: Rename last method to get_v05 nomenclature

astuyve added 6 commits June 17, 2024 14:19

feat: add Lambda

b92f4eb

feat: env verifier

71fd0f8

feat: set origin tag correctly

e37b91e

debug: what is v5 sending us

53e37af

revert

dd1630c

feat: v0.5 trace decompression support

bdc97be

astuyve requested review from a team as code owners June 25, 2024 20:30

github-actions bot added the mini-agent label Jun 25, 2024

astuyve added 2 commits June 25, 2024 16:34

fix: remove comment and unwraps

36a153b

Merge branch 'main' into aj/bottlecap-mini-agent

c26a72a

astuyve added 2 commits June 25, 2024 16:37

feat: remove erroneous GCP comment from my copypasta

b47968f

fix: complex type, license for rmpv

942f186

duncanista reviewed Jun 25, 2024

View reviewed changes

trace-utils/src/trace_utils.rs Outdated Show resolved Hide resolved

duncanista reviewed Jun 25, 2024

View reviewed changes

trace-utils/src/trace_utils.rs Outdated Show resolved Hide resolved

duncanista reviewed Jun 25, 2024

View reviewed changes

trace-utils/src/trace_utils.rs Outdated Show resolved Hide resolved

duncanista reviewed Jun 25, 2024

View reviewed changes

trace-utils/src/trace_utils.rs Show resolved Hide resolved

duncanista approved these changes Jun 25, 2024

View reviewed changes

morrisonlevi reviewed Jun 25, 2024

View reviewed changes

bantonsson requested changes Jun 26, 2024

View reviewed changes

astuyve added 4 commits June 26, 2024 12:35

feat: Rename methods to use v05. Add v05 string method. Temp debuggin…

6bcdc05

…g log to figure out json string encoding stuff

wip: more debugging

b904c3a

fix: use into_str instead of to_string to avoid string escaping

4e4b8bc

feat: clean up match

5aebb90

astuyve requested a review from bantonsson June 26, 2024 17:38

bantonsson approved these changes Jun 27, 2024

View reviewed changes

feat: Rename last method to get_v05 nomenclature

70e74a6

Merge branch 'main' into aj/bottlecap-mini-agent

e650b86

astuyve merged commit 2d7534d into main Jun 27, 2024
32 checks passed

astuyve deleted the aj/bottlecap-mini-agent branch June 27, 2024 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support v0.5 trace endpoint #505

Support v0.5 trace endpoint #505

astuyve commented Jun 25, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading

duncanista Jun 25, 2024

astuyve Jun 25, 2024

astuyve Jun 25, 2024

bantonsson Jun 26, 2024

duncanista left a comment

morrisonlevi Jun 25, 2024

astuyve Jun 25, 2024

bantonsson Jun 26, 2024

bantonsson left a comment

bantonsson Jun 26, 2024

bantonsson Jun 26, 2024

bantonsson Jun 26, 2024

astuyve Jun 26, 2024

bantonsson Jun 26, 2024

bantonsson Jun 27, 2024

astuyve Jun 27, 2024

pr-commenter bot commented Jun 27, 2024

Group 1

Warnings

Group 1

Warnings

Support v0.5 trace endpoint #505

Support v0.5 trace endpoint #505

Conversation

astuyve commented Jun 25, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

How to test the change?

codecov-commenter commented Jun 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncanista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantonsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr-commenter bot commented Jun 27, 2024

Benchmarks

Comparison

Parameters

Summary

Candidate

Group 1

Warnings

Baseline

Group 1

Warnings

astuyve commented Jun 25, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading