Add priority sampling #169
Conversation
labbati
force-pushed
the
labbati/priority-sampling
branch
from
7d947b3 to
c5bbde1
Compare
src/DDTrace/Transport/Http.php
Outdated
| * @param array $traces | ||
| * @return bool | ||
| */ | ||
| private function isPrioritySamplingEndpoint(array $traces) |
There was a problem hiding this comment.
isPrioritySamplingEndpoint seems quite specific to endpoints, which is only partialy true, because method body doesn't mention endpoints but its only used there.
As such, would isPrioritySamplingUsed work here ? -< its a bit more generic IMO
src/DDTrace/Transport/Http.php
Outdated
| $path = self::DEFAULT_TRACE_AGENT_PATH; | ||
| $endpoint = "http://${host}:${port}${path}"; | ||
| $defaultEndpoint = "http://${host}:${port}" . self::DEFAULT_TRACE_AGENT_PATH; | ||
| $prioritySamlpingEndpoint = "http://${host}:${port}" . self::PRIORITY_SAMPLING_TRACE_AGENT_PATH; |
There was a problem hiding this comment.
I know this is probably not very bad for performance.
But it worries me a bit that we're doing some computations every request, that are not normally used during the request and after.
Overall would be great to defer most of the transport setup to the very end when we need to actually send (but this is probably out of the scope for this PR).
There was a problem hiding this comment.
This is a good point and I like the idea. If you agree I will do for the parts that I touched in this PR and will postpone a wider refactoring of the Http transport (as the same applies also to other config params) to a later stage.
There was a problem hiding this comment.
As discussed, it is risky as it introduces potential breaking changes for users, so I would defer it to a later http transport refactoring
There was a problem hiding this comment.
Thanks @pawelchcki for the first round of CR. I answered to your comments
src/DDTrace/Transport/Http.php
Outdated
| $path = self::DEFAULT_TRACE_AGENT_PATH; | ||
| $endpoint = "http://${host}:${port}${path}"; | ||
| $defaultEndpoint = "http://${host}:${port}" . self::DEFAULT_TRACE_AGENT_PATH; | ||
| $prioritySamlpingEndpoint = "http://${host}:${port}" . self::PRIORITY_SAMPLING_TRACE_AGENT_PATH; |
There was a problem hiding this comment.
This is a good point and I like the idea. If you agree I will do for the parts that I touched in this PR and will postpone a wider refactoring of the Http transport (as the same applies also to other config params) to a later stage.
src/DDTrace/Transport/Http.php
Outdated
| * @param array $traces | ||
| * @return bool | ||
| */ | ||
| private function isPrioritySamplingEndpoint(array $traces) |
labbati
force-pushed
the
labbati/priority-sampling
branch
from
550fc7b to
723580d
Compare
|
@pawelchcki I did some manual testing and look good. It would be great to have your latest CR round. Thanks a lot! |
Description
This PR adds priority sampling management to the game. In the process, some refactoring to the way we handle distributed tracing in general was necessary.
Readiness checklist