-
Notifications
You must be signed in to change notification settings - Fork 375
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
Create priority sampler feedback loop #249
Create priority sampler feedback loop #249
Conversation
548c403
to
f73cc15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, needs a real-world try and CI fixes maybe, but approach is good.
@@ -20,30 +20,35 @@ class HTTPTransport | |||
RUBY_INTERPRETER = RUBY_VERSION > '1.9' ? RUBY_ENGINE + '-' + RUBY_PLATFORM : 'ruby-' + RUBY_PLATFORM | |||
|
|||
API = { | |||
'v0.3' => { | |||
V4 = 'v0.4'.freeze => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -176,5 +183,15 @@ def stats | |||
} | |||
end | |||
end | |||
|
|||
private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, private, indeed.
test/writer_test.rb
Outdated
def test_sampling_feedback_loop | ||
sampler = Minitest::Mock.new | ||
writer = Writer.new(priority_sampler: sampler) | ||
response_body = { 'service_a' => 0.1, 'service_b' => 0.5 }.to_json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Real-world responses are more like:
{ 'service:,env:' => 0.1, 'service:a,env:test' => 0.3, 'service:b,env:test' => 0.15 }
while, technically, it's perfectly fine to test it with the entry you used, I also think it's good that the test data, be it mocked, actually reflect real use cases, so it would be nice to have at least one test that has real data (or at least, data that is technically likely to happen). On the agent you get hints on this here https://github.com/DataDog/datadog-trace-agent/blob/master/sampler/catalog_test.go#L66 or there https://github.com/DataDog/datadog-trace-agent/blob/master/agent/info_test.go#L107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I agree 100%
f73cc15
to
82c9628
Compare
82c9628
to
1c3cda0
Compare
This PR provides the feedback loop between the
datadog-trace-agent
and thePrioritySampler
. The purpose of it is to adjust dynamically the sampling rates of each service after each flush.