Possibility to enable/disable distributed tracing and priority sampling #160
Conversation
labbati
force-pushed
the
labbati/config-disable-distributed-tracing
branch
from
0c54af0 to
7a29d03
Compare
labbati
changed the title
labbati
force-pushed
the
labbati/config-disable-distributed-tracing
branch
2 times, most recently
from
08fbf2d to
f9d5a5f
Compare
labbati
force-pushed
the
labbati/config-disable-distributed-tracing
branch
from
f9d5a5f to
b2e0f9d
Compare
| * Registry that holds configuration properties and that is able to recover configuration values from environment | ||
| * variables. | ||
| */ | ||
| class EnvVariableRegistry |
There was a problem hiding this comment.
Would it make sense to extract interface of this?
| * | ||
| * @return bool | ||
| */ | ||
| public function isDistributedTracingEnabled() |
There was a problem hiding this comment.
I'm stumped here a bit. one hand I really like that we're using statically analyzable code to fetch configuration.
OTOH, adding a method for every configuration option - I feel like it might be a bit of an overkill in the long run.
WDYT if AbstractConfiguration implemented also VariableRegistry interface.
So that boolValue could be reimplemented here.
And we could use this configuration like so:
$globalConfiguration->boolValue('distributed.tracing');
Hmm. This also means that we would have split the configuration defaults from the implementation. Maybe to a DefaultVariableRegistry ?
I might be optimizing a bit for the future here, so just thinking out loud. Otherwise I really like this approach!
There was a problem hiding this comment.
Ehehehe the solution that you proposed it much more php-style, so I would be totally for that.
To be honest, I really like to have statically analysable methods and I don't think that they will be dozens.
So I still gives my vote to methods 😄 but totally opened to change it if you guys prefer 😄
There was a problem hiding this comment.
I have a feeling we'll end up with both, and it should be cool too. I.e. the big things like isDistrubutedTracingEnabled() is really good case for methods approach.
The small things like:
agent.connection.timeoutagent.send.timeoutagent.portagent.hostname
Might work better as string addressable config.
There was a problem hiding this comment.
TBH I am not a big fan of allowing both 😄 but I also acknowledge that the string approach is more in line with PHP coding styles. So let me add both the access mechanism...but I hope that everybody will use the explicit method call 😄.
|
@pawelchcki I applied changes from your comments, it would be great if you have some time to take it a look for second round of code review. |
src/DDTrace/Configuration.php
Outdated
| */ | ||
| public function isDistributedTracingEnabled() | ||
| { | ||
| return $this->registry->boolValue('distributed.tracing', true); |
There was a problem hiding this comment.
Can you use $this->boolValue(... here ?
There was a problem hiding this comment.
eheheh very good catch!!! 👍 thanks
Description
This PR adds the possibility to enable and disable distributed tracing and priority sampling
Readiness checklist