Skip to content

ddtrace/tracer: removed exactService and exactName from sampling rules#2416

Merged
dianashevchenko merged 10 commits into
shevchenko/tag-resource-samplingfrom
shevchenko/update-trace-rules-to-glob
Dec 19, 2023
Merged

ddtrace/tracer: removed exactService and exactName from sampling rules#2416
dianashevchenko merged 10 commits into
shevchenko/tag-resource-samplingfrom
shevchenko/update-trace-rules-to-glob

Conversation

@dianashevchenko

@dianashevchenko dianashevchenko commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

This test removes exactService and exactName, in favour of just Service and Name fields in the SamplingRule

Motivation

Spotted inconsistency when running system-tests

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.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.

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.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@pr-commenter

pr-commenter Bot commented Dec 11, 2023 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2023-12-11 14:43:05

Comparing candidate commit 80ad819 in PR branch shevchenko/update-trace-rules-to-glob with baseline commit 50bcd69 in branch shevchenko/extend-span-sampling.

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

knusbaum
knusbaum reviewed Dec 11, 2023
View reviewed changes

@knusbaum knusbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we do a quick benchmark for this stuff?

It would be nice to have an idea (for this change and the others) what the costs might be.

@dianashevchenko

dianashevchenko commented Dec 12, 2023

Copy link
Copy Markdown
Contributor Author

Yes, will do to compare with main, I guess 🙂

Base automatically changed from shevchenko/extend-span-sampling to shevchenko/tag-resource-sampling December 18, 2023 16:39
@dianashevchenko
dianashevchenko marked this pull request as ready for review December 18, 2023 16:39
@dianashevchenko
dianashevchenko requested a review from a team December 18, 2023 16:39
@darccio

darccio commented Dec 19, 2023

Copy link
Copy Markdown
Member

@dianashevchenko Where I can read a bit more of context? I'm having some difficulties to understand what inconsistencies lead to these changes. Thanks!

@dianashevchenko
dianashevchenko merged commit e13e8d7 into shevchenko/tag-resource-sampling Dec 19, 2023
@dianashevchenko
dianashevchenko deleted the shevchenko/update-trace-rules-to-glob branch December 19, 2023 13:54
@dianashevchenko

dianashevchenko commented Dec 19, 2023

Copy link
Copy Markdown
Contributor Author

@darccio working according to the latest sampling RFC + there was a new set of tests introduced recently, that's not enabled in CI yet for which these changes are necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knusbaum knusbaum knusbaum left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dianashevchenko @darccio @knusbaum