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

feat(datadog): add sampling priority tag in datadog spans #792

Merged
merged 4 commits into from
Jun 10, 2022

Conversation

rex-remind101
Copy link
Contributor

encode sampling priority into metrics object of data dog exported data. data dog uses this to sample.

solves #791

@rex-remind101 rex-remind101 requested a review from a team as a code owner May 11, 2022 01:10
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 11, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rex-remind101 / name: Rex (3f48047)

@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #792 (e1e9b0c) into main (1f84f4a) will decrease coverage by 0.0%.
The diff coverage is 70.5%.

@@           Coverage Diff           @@
##            main    #792     +/-   ##
=======================================
- Coverage   69.7%   69.7%   -0.1%     
=======================================
  Files        110     110             
  Lines       9061    9072     +11     
=======================================
+ Hits        6324    6331      +7     
- Misses      2737    2741      +4     
Impacted Files Coverage Δ
opentelemetry-datadog/src/exporter/model/v03.rs 85.4% <66.6%> (-1.6%) ⬇️
opentelemetry-datadog/src/exporter/model/v05.rs 82.3% <71.4%> (-1.6%) ⬇️
opentelemetry-datadog/src/exporter/model/mod.rs 77.3% <100.0%> (ø)
opentelemetry-http/src/lib.rs 7.0% <0.0%> (-5.3%) ⬇️
opentelemetry-sdk/src/trace/span_processor.rs 79.7% <0.0%> (-0.1%) ⬇️
opentelemetry-sdk/src/trace/runtime.rs 15.7% <0.0%> (+0.3%) ⬆️
opentelemetry-sdk/src/metrics/controllers/push.rs 83.3% <0.0%> (+3.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f84f4a...e1e9b0c. Read the comment docs.

@TommyCpp
Copy link
Contributor

TommyCpp commented May 24, 2022

We need to format the code by cargo fmt

@TommyCpp TommyCpp changed the title [dd-metrics-encode] feat(datadog): add sampling priority tag in datadog spans May 31, 2022
rmp::encode::write_map_len(&mut encoded, 1)?;
// https://github.com/DataDog/dd-trace-js/blob/c89a35f7d27beb4a60165409376e170eacb194c5/packages/dd-trace/src/constants.js#L4
rmp::encode::write_u32(&mut encoded, interner.intern("_sampling_priority_v1"))?;
rmp::encode::write_f64(&mut encoded, if span.span_context.is_sampled() { 1.0 } else { 0.0 })?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we define some constant at model/mod.rs and reuse them across v3 and v5 exporter?

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Have one nit and need to format the code but otherwise LGTM. Thanks for the help!

@rex-remind101
Copy link
Contributor Author

rex-remind101 commented Jun 1, 2022

amended my commit with the suggestions. attempted to use the EasyCLA but I'm receiving the following The Individual CLA option is not enabled for this project. Please to your administrator to enable the Individual CLA option for this project. so i'm not sure how to move forward on this.

@TommyCpp
Copy link
Contributor

TommyCpp commented Jun 2, 2022

The Individual CLA option is not enabled for this project. Please to your administrator to enable the Individual CLA option for this project

Could you take a screenshot? It should be something like
image

@rex-remind101
Copy link
Contributor Author

it worked this time

…ata dog exported data. data dog uses this to sample.
@TommyCpp
Copy link
Contributor

TommyCpp commented Jun 3, 2022

@rex-remind101 I think we need to bump the msrv to 1.56 to resolve the CI issues. Could you make the change in https://github.com/open-telemetry/opentelemetry-rust/blob/main/.github/workflows/ci.yml#L71 ?

I don't have the write access to the branch.

@rex-remind101
Copy link
Contributor Author

@TommyCpp ready!

@rex-remind101
Copy link
Contributor Author

@TommyCpp looks like there is still an error actually, could you help provide direction?

@TommyCpp
Copy link
Contributor

TommyCpp commented Jun 9, 2022

@TommyCpp looks like there is still an error actually, could you help provide direction?

I think the problem is still the MSRV. Let me experiment with some versions and see which one we need to use.

Confirmed 1.59 works locally for me so I think updating the MSRV from 1.56 to 1.59 will fix the issue

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM. Thanks!

@TommyCpp TommyCpp merged commit 3806258 into open-telemetry:main Jun 10, 2022
@rex-remind101 rex-remind101 deleted the dd-metrics-encode branch June 10, 2022 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants