Configure sampling strategies #139
Conversation
|
This change is |
pkg/strategy/production_test.go
Outdated
| } | ||
| assert.Equal(t, 3, escount) | ||
| // TODO: Added break as deployments includes both the original and modified |
There was a problem hiding this comment.
Not clear why the original and modified deployments are both being returned by getDeployments(objs) aibove?
There was a problem hiding this comment.
Sorry my mistake - were for different deployments 😄
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
==========================================
+ Coverage 96.16% 96.21% +0.04%
==========================================
Files 27 28 +1
Lines 1201 1267 +66
==========================================
+ Hits 1155 1219 +64
- Misses 36 37 +1
- Partials 10 11 +1
Continue to review full report at Codecov.
|
Signed-off-by: Gary Brown <[email protected]>
Signed-off-by: Gary Brown <[email protected]>
Signed-off-by: Gary Brown <[email protected]>
|
@jpkrohling Would it be possible to review this one soon - there is an issue highlighted - not sure if you have an answer straightaway, whether it should be handle in a separate PR, etc? If this one may not be merged soon, would it be possible to do a release with the secrets support today? |
Signed-off-by: Gary Brown <[email protected]>
There was a problem hiding this comment.
There are a couple of minor things, but nothing blocking. Feel free to merge if you prefer to address them later.
Reviewed 7 of 13 files at r1, 7 of 7 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @objectiser)
README.adoc, line 307 at r3 (raw file):
options: default_strategy: type: "probabilistic"
The quotes aren't needed, are they?
README.adoc, line 311 at r3 (raw file):
---- This example defines a default sampling strategy that is probabilitic, with 50% of the trace instances being
s/probabilitic/probabilistic/
As it's "probabilistic", saying that 50% will be traced is technically wrong. "With a 50% chance that ..." would be more accurate.
README.adoc, line 314 at r3 (raw file):
sampled. Refer to the Jaeger documentation on link:https://www.jaegertracing.io/docs/1.8/sampling/#collector-sampling-configuration[Collector Sampling Configuration] to see how service and endpoint sampling can be configured. The json representation described in that documentation can be used in the operator by converting to yaml.
s/json/JSON/ ?
s/yaml/YAML/ ?
deploy/examples/with-sampling.yaml, line 10 at r3 (raw file):
options: default_strategy: type: "probabilistic"
If you change the readme to remove the quotes, remove it from here as well
pkg/config/sampling/sampling.go, line 43 at r3 (raw file):
} logrus.Debug("Assembling the Sampling configmap")
Might be worth adding a field with the Jaeger instance name, in case there are more instances being installed at the same time.
logrus.WithField("instance", jaeger.Name).Debug(...)
pkg/config/sampling/sampling_test.go, line 15 at r3 (raw file):
config := NewConfig(jaeger) dep := config.Get()
dep == deployment. I think this is a ConfigMap.
pkg/config/sampling/sampling_test.go, line 28 at r3 (raw file):
dep := config.Get() assert.NotNil(t, dep) assert.Equal(t, defaultSamplingStrategy, dep.Data["sampling"])
We have a ConfigMap with the sampling config by default now, I assume.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @objectiser and @jpkrohling)
README.adoc, line 307 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
The quotes aren't needed, are they?
no
README.adoc, line 311 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
s/probabilitic/probabilistic/As it's "probabilistic", saying that 50% will be traced is technically wrong. "With a 50% chance that ..." would be more accurate.
Done.
README.adoc, line 314 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
s/json/JSON/?
s/yaml/YAML/?
Done.
deploy/examples/with-sampling.yaml, line 10 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
If you change the readme to remove the quotes, remove it from here as well
Done.
pkg/config/sampling/sampling.go, line 43 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Might be worth adding a field with the Jaeger instance name, in case there are more instances being installed at the same time.
logrus.WithField("instance", jaeger.Name).Debug(...)
Done.
pkg/config/sampling/sampling_test.go, line 15 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
dep== deployment. I think this is aConfigMap.
Done.
pkg/config/sampling/sampling_test.go, line 28 at r3 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
We have a
ConfigMapwith the sampling config by default now, I assume.
Yes
Signed-off-by: Gary Brown <[email protected]>
Signed-off-by: Gary Brown <[email protected]>
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4.
Dismissed @objectiser from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved
Resolves #83
There is an issue in
pkg/strategy/production_test.gothat I am not sure about.Once this PR is merged, if
pkg/config/samplingpackage structure is accepted, then I will do a separate PR to refactor thepkg/configmappackage o bepkg/config/ui.Signed-off-by: Gary Brown [email protected]