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

Create priority sampler feedback loop #249

Merged

Conversation

p-lambert
Copy link
Member

@p-lambert p-lambert commented Nov 15, 2017

This PR provides the feedback loop between the datadog-trace-agent and the PrioritySampler. The purpose of it is to adjust dynamically the sampling rates of each service after each flush.

@p-lambert p-lambert changed the base branch from master to christian/service-sampler November 15, 2017 23:10
@p-lambert p-lambert changed the base branch from christian/service-sampler to pedro/service-sampler November 15, 2017 23:10
@p-lambert p-lambert force-pushed the pedro/enable-distributed-sampling-on-transport branch 2 times, most recently from 548c403 to f73cc15 Compare November 16, 2017 15:02
@p-lambert p-lambert added the core Involves Datadog core libraries label Nov 16, 2017
@p-lambert p-lambert added this to the 0.10.0 milestone Nov 16, 2017
Copy link
Contributor

@ufoot ufoot left a 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 => {
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, private, indeed.

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
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I agree 100%

@p-lambert p-lambert force-pushed the pedro/enable-distributed-sampling-on-transport branch from f73cc15 to 82c9628 Compare November 20, 2017 21:05
@p-lambert p-lambert force-pushed the pedro/enable-distributed-sampling-on-transport branch from 82c9628 to 1c3cda0 Compare November 21, 2017 15:45
@palazzem palazzem merged commit 980fb20 into pedro/service-sampler Nov 21, 2017
@palazzem palazzem deleted the pedro/enable-distributed-sampling-on-transport branch November 21, 2017 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants