Implemented random sampling #116
Conversation
beniwohli
force-pushed
the
feature/transaction-sampling
branch
4 times, most recently
from
77729ce
to
31701b0
Compare
beniwohli
force-pushed
the
feature/transaction-sampling
branch
from
31701b0
to
511f36b
Compare
beniwohli
force-pushed
the
feature/transaction-sampling
branch
from
511f36b
to
30b05f2
Compare
elasticapm/traces.py
Outdated
| @@ -230,14 +244,14 @@ def decorated(*args, **kwds): | |||
| def __enter__(self): | |||
| transaction = get_transaction() | |||
|
|
|||
| if transaction: | |||
| if transaction and transaction.is_sampled: | |||
| transaction.begin_span(self.name, self.type, self.extra, | |||
There was a problem hiding this comment.
Do you need the check for in_sampled it both here and inside begin_span?
| @@ -219,7 +219,8 @@ def uninstrument(self): | |||
| self.originals = {} | |||
|
|
|||
| def call_if_sampling(self, module, method, wrapped, instance, args, kwargs): | |||
| if not get_transaction(): | |||
| transaction = get_transaction() | |||
| if not transaction or not transaction.is_sampled: | |||
| return wrapped(*args, **kwargs) | |||
There was a problem hiding this comment.
you have the check in begin_span. Do you need it here too?
There was a problem hiding this comment.
not 100% sure tbh. I'll step through it with the debugger
There was a problem hiding this comment.
After taking a closer read of the code, I think it's worth doing the check in both places.
call_if_sampling is called for stuff we instrument automatically. Checking here avoids even calling the capture_span decorator and doing other stuff we do in the wrappers (e.g. parsing the SQL query for an sql span).
the check in capture_span.__enter__ is useful for cases where code is instrumented manually using the capture_span decorator.
The third check, in Transaction.begin_span seems to be superfluous, however, as it is only called from capture_span.__enter__. Good catch!
elasticapm/traces.py
Outdated
| @@ -102,7 +112,8 @@ def to_dict(self): | |||
| 'context': self._context, | |||
| 'spans': [ | |||
There was a problem hiding this comment.
I think it's worth considering removing the spans key entirely to clearly indicate this is an unsampled transaction.
beniwohli
force-pushed
the
feature/transaction-sampling
branch
from
30b05f2
to
5e49a5d
Compare
Given a sample rate, this will randomly choose transactions to be sampled. If a transaction isn't sampled, no spans will be collected, as well as no context information
beniwohli
force-pushed
the
feature/transaction-sampling
branch
from
5e49a5d
to
4dee914
Compare
Given a sample rate, this will randomly choose transactions to be sampled. If a transaction isn't sampled, no spans will be collected, as well as no context information closes #116
Given a sample rate, this will randomly choose transactions
to be sampled. If a transaction isn't sampled, no spans will
be collected, as well as no context information