Add sampling handler and sampling strategy store#674
Conversation
black-adder
requested review from
pavolloffay,
vprithvi and
yurishkuro
as code owners
ghost
assigned
| ) | ||
|
|
||
| const ( | ||
| samplingStrategies = "sampling.strategies" |
There was a problem hiding this comment.
I would prefer a bit more explicit name "sampling.strategies-file"
| return h.defaultStrategy, nil | ||
| } | ||
|
|
||
| // TODO good candidate for a global util function |
There was a problem hiding this comment.
how much overlap is in this code with what the client is doing when parsing JSON response from the agent? Maybe we can combine that all into jaeger-lib?
There was a problem hiding this comment.
This TODO was originally meant for creating a util function for reading a json file and unmarshalling it. I didn't have as lofty goal of moving the client and this logic into jaeger-lib. I don't think it's similar enough to warrant that.
|
@black-adder does this fix #661? |
Signed-off-by: Won Jun Jang <wjang@uber.com>
black-adder
force-pushed
the
add_sampling_handler
branch
from
48ad22e to
c34fa8b
Compare
Signed-off-by: Won Jun Jang <wjang@uber.com>
Signed-off-by: Won Jun Jang <wjang@uber.com>
| @@ -0,0 +1,44 @@ | |||
| // Copyright (c) 2018 The Jaeger Authors. | |||
There was a problem hiding this comment.
yes, the pkg is rather suspicious for this. It is meant for things not directly related to Jaeger. The simplest approach would be to define the storage interface in cmd/collector/sampling/strategystore/interface.go
There was a problem hiding this comment.
done, should I have the static/adaptive implementations in plugin or should I move the whole thing over to cmd/collector/app?
| // | ||
| // See also | ||
| // | ||
| // plugin.Configurable |
There was a problem hiding this comment.
Configurable also has AddFlags(flagSet *flag.FlagSet). Do you actually need to require Configurable methods in the Factory interface?
There was a problem hiding this comment.
It doesn't have to be but it makes life easier.
| "github.com/jaegertracing/jaeger/thrift-gen/sampling" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
is this needed in the "interface" package? Could it not be placed in the static impl?
|
|
||
| // Strategy defines a sampling strategy. Type can be "probabilistic" or "ratelimiting" | ||
| // and Param will represent "sampling probability" and "max traces per second" respectively. | ||
| type Strategy struct { |
There was a problem hiding this comment.
these types look like private intermediate model used by static impl. Or did you mean the StrategyStorage interface to return these?
| // FactoryConfigFromEnv reads the desired sampling type from the SAMPLING_TYPE environment variable. Allowed values: | ||
| // * `static` - built-in | ||
| // * `adaptive` - built-in // TODO | ||
| func FactoryConfigFromEnv() FactoryConfig { |
There was a problem hiding this comment.
are you planning to add changes to collector.main() here? Right now this function is not used.
There was a problem hiding this comment.
I'm going to do that in the next PR, this one for API
Signed-off-by: Won Jun Jang <wjang@uber.com>
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
check why code coverage drops
4 participants
This adds a tchannel sampling handler and a static sampling strategy store that reads sampling strategies from a json file.
I added a factory for this so that when we move adaptive sampling over to open source, we can easily extend the existing factory with adaptive sampling and all its flags (I've counted 15 flags at the time of writing).
Locations of files might be up for debate (should this belong in pkg and plugin?)
Do I even need a separate sampling strategy store? should I just use the sampling handler interface and have a factory for that?
Fixes #661
Signed-off-by: Won Jun Jang wjang@uber.com