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

Proposal: create metrics from traces and agent based sampling #17

Merged
merged 14 commits into from
Mar 20, 2024

Conversation

Hartigan
Copy link
Contributor

@Hartigan Hartigan commented Dec 3, 2023

Hi

This fraft pull request introduce 2 features from datadog telemetry protocol:

  1. Measurements - if _dd.measured flag present in span, datadog agent automatically creates metrics from this span.
  2. Agent based sampling - by default ALL spans generated by dd-trace-agent exported to trace-agent. And agent decide export span or not to Datadog cloud service. This decision based on _sampling_priority_v1 flag and trace-agent configuration(ex. force to sample error spans).

Described flags, may be propagated or changed by ShouldSample interface

Current code not working, because decision about export trace or not created here

Copy link

linux-foundation-easycla bot commented Dec 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@cijothomas
Copy link
Member

@Hartigan Could you clear the EasyCLA check before we can accept any contributions? You may want to consult your employer's legal team, if needed.

@Hartigan
Copy link
Contributor Author

Hartigan commented Dec 8, 2023

Hi @cijothomas

Should I change smth in this pull request(api or add custom implementation of ShouldSample)? Or write some tests for this?

@cijothomas
Copy link
Member

Hi @cijothomas

Should I change smth in this pull request(api or add custom implementation of ShouldSample)? Or write some tests for this?

I am not an expert in this component, so can't give a specific suggestion. I hope there are DataDog experts or other owners of this component who can help.

@TommyCpp
Copy link
Contributor

TommyCpp commented Dec 8, 2023

Some unit tests are always welcome!

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: Patch coverage is 84.42623% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 52.4%. Comparing base (e23abe4) to head (f4a3737).

Files Patch % Lines
opentelemetry-datadog/src/lib.rs 83.9% 17 Missing ⚠️
opentelemetry-datadog/src/exporter/model/v05.rs 87.5% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main     #17     +/-   ##
=======================================
+ Coverage   51.7%   52.4%   +0.6%     
=======================================
  Files         37      37             
  Lines       4918    5026    +108     
=======================================
+ Hits        2544    2635     +91     
- Misses      2374    2391     +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hartigan Hartigan marked this pull request as ready for review January 4, 2024 20:09
@Hartigan Hartigan requested a review from a team January 4, 2024 20:09
@hdost hdost changed the title Proposal: create metrics from traces and agent based samling Proposal: create metrics from traces and agent based sampling Jan 16, 2024
Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, also make sure to add a comment into the CHANGELOG.md.

opentelemetry-datadog/Cargo.toml Show resolved Hide resolved
@cijothomas
Copy link
Member

@Hartigan Could you resolve conflicts? Thanks!

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Not an expert in this particular component, so will wait another day before merge to see if anyone else can help with additional reviews.

@cijothomas
Copy link
Member

@Hartigan could you resolve conflicts, so we can merge?

@cijothomas cijothomas closed this Mar 20, 2024
@cijothomas cijothomas reopened this Mar 20, 2024
@cijothomas cijothomas merged commit a84e207 into open-telemetry:main Mar 20, 2024
15 checks passed
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.

4 participants