dd-trace-go icon indicating copy to clipboard operation
dd-trace-go copied to clipboard

ddtrace/tracer: Golang tracer support for remote sampling rules

Open yuanyuanzhao3 opened this issue 1 year ago • 2 comments
trafficstars

DNMC-21

What does this PR do?

Supports remote trace sampling rules. Also reports sampling rules (local or remote) through instrumentation telemetry for migration.

Motivation

Reviewer's Checklist

  • [ ] Changed code has unit tests for its functionality at or near 100% coverage.
  • [ ] System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • [ ] There is a benchmark for any new code, or changes to existing code.
  • [ ] If this interacts with the agent in a new way, a system test has been added.
  • [X] Add an appropriate team label so this PR gets put in the right place for the release notes.
  • [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

For Datadog employees:

  • [ ] If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • [X] This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

yuanyuanzhao3 avatar Mar 06 '24 17:03 yuanyuanzhao3

FYI, this is blocked behind a Golang tracer issue on output json of the sampling rule: https://dd.slack.com/archives/CKXKHAKFG/p1709736745814879

A new test thus fails without a fix to that problem. This PR is shared so we can take a look in parallel.

yuanyuanzhao3 avatar Mar 06 '24 17:03 yuanyuanzhao3

Benchmarks

Benchmark execution time: 2024-03-15 13:16:17

Comparing candidate commit c42676ad1c0e6a904f4f1a1340075361429cf49d in PR branch yuanyuan.zhao/remote-resource-tag-sampling-rules with baseline commit 7bde0099d183af7b3d4303cdbd5244cb65115834 in branch main.

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

pr-commenter[bot] avatar Mar 06 '24 18:03 pr-commenter[bot]

@yuanyuanzhao3 hi there, have you looked into why the changes cause the increase in StartSpan time?

dianashevchenko avatar Mar 13 '24 13:03 dianashevchenko

@yuanyuanzhao3 hi there, have you looked into why the changes cause the increase in StartSpan time?

Hi, I do not get a message that there was an increase in StartSpan time. The relevant email I got on performance is this:

Benchmarks Benchmark execution time: 2024-03-06 18:02:13

Comparing candidate commit 0ec8d94 in PR branch yuanyuan.zhao/remote-resource-tag-sampling-rules with baseline commit 63d7047 in branch main.

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

Can you let me know where that StartSpan increase was reported? Thanks.

yuanyuanzhao3 avatar Mar 13 '24 14:03 yuanyuanzhao3

I am referring to the BenchmarkStartSpan, it was previously reported by the benchmarking bot. I see that the message is now edited and is no longer a case(you can see it in the history of the message ) With that said, I would recommend double checking just in case. image

dianashevchenko avatar Mar 13 '24 14:03 dianashevchenko

I am referring to the BenchmarkStartSpan, it was previously reported by the benchmarking bot. I see that the message is now edited and is no longer a case(you can see it in the history of the message ) With that said, I would recommend double checking just in case. image

Sounds good. I'll definitely check.

Also, it seems that there are PRs on the json marshaling change merged? Is it ready for me to pull in prod and update this PR? Just want to know so I go over it and check the benchmark together.

yuanyuanzhao3 avatar Mar 13 '24 14:03 yuanyuanzhao3

Overall, looks good to me. Is there testing supplementing this code on the system-tests side?

Yeah. Misha is getting someone from the Python team to do it.

yuanyuanzhao3 avatar Mar 14 '24 17:03 yuanyuanzhao3