implement client sampling #74
Conversation
galdor
force-pushed
the
nicolas/client-sampling
branch
2 times, most recently
from
91f4c20
to
778821f
Compare
galdor
force-pushed
the
nicolas/client-sampling
branch
from
778821f
to
c6ca9f0
Compare
lib/ddtrace/sampler.rb
Outdated
| module Datadog | ||
| # \Sampler performs client-side trace sampling. | ||
| class Sampler | ||
| def sample(span); end |
There was a problem hiding this comment.
do we need this class as a kind of interface? I assume users can do Datadog::Sampler.new().sample(span) (which is wrong) so I think it should raise an error like:
class Sampler
def sample(span);
raise 'You have to implement this method' # or whatever
end
endObviously if it's Ruby-idiomatic.
lib/ddtrace/sampler.rb
Outdated
| # sampled. | ||
| def initialize(sample_rate = 1.0) | ||
| unless sample_rate > 0.0 && sample_rate <= 1.0 | ||
| raise ArgumentError, |
There was a problem hiding this comment.
this exception is handled somewhere? does it crash users code? an alternative is to ignore the sample_rate value, disabling the Sampler and logging the error.
There was a problem hiding this comment.
As I already explained, raising an exception is not a crash. ArgumentError exceptions are designed very precisely for this kind of problem. Logging errors and disabling the sampler is very bad because people miss errors, and then believe the code works as they intended to... even though it does not. If they use an invalid sample rate, the code will raise an exception, which they won't be able to miss, and they will be able to fix it before going to prod. In C you would use an assertion, with the exact same purpose.
There was a problem hiding this comment.
In that case we should update also the dd-trace-py implementation: https://github.com/DataDog/dd-trace-py/blob/e96fa4837f84e6c466262bd3d3d6df9cb912aa9a/ddtrace/sampler.py#L32-L36
@LotharSee thoughts?
There was a problem hiding this comment.
While I agree with raising an error in the context of an important/business piece of code, we should shoot for something more conservative as an monitoring/instrumentation lib.
The same way as we don't raise exceptions if the Agent endpoint is wrong, if one does something wrong with a span, ... we should behave as a 0-footprint best-effort lib.
And in general, as a user I'd be worried if my APM solution can break my app.
lib/ddtrace/span.rb
Outdated
| @metrics[key] = value | ||
| end | ||
|
|
||
| # Return the metric wth the given key, nil if it doesn't exist. |
| @@ -40,9 +40,11 @@ def initialize(tracer, name, options = {}) | |||
| @trace_id = options.fetch(:trace_id, @span_id) | |||
|
|
|||
| @meta = {} | |||
| @metrics = {} | |||
There was a problem hiding this comment.
That was missing so good to have it!
In that case you should update all our tests so that we have both unittests and integration tests with a real agent. We should check also what happens if we put strings, nil values, integers, etc...
galdor
force-pushed
the
nicolas/client-sampling
branch
from
c6ca9f0
to
51c3c12
Compare
galdor
force-pushed
the
nicolas/client-sampling
branch
from
51c3c12
to
950990c
Compare
|
Tests will be part of another PR, merging in the meantime. |
No description provided.