Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[trace] Add optional flags to support flexible jaeger configs #8199

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented May 28, 2021

Closes #8198.

Description

A common pain point with the standard package flag is the inability to
tell whether a flag was set on the command line or not. You can check
against the default value, but that's not perfect either, because then
you cannot tell the difference between a flag that was not specified at
all and a flag that was set to exactly the default value.

Enter OptionalFlag. We define some structs that wrap a primitive
(float64 for example) with a boolean flag to track whether Set() was
ever called on the flag. Since, for the jaeger sampler params, we only
need a string for the sampler type and a float64 for the sampling
rate, I only implemented OptionalFlag for those two types for now.
Plus, all of this becomes obsolete if we move to cobra, so I wasn't
highly-motivated to implement this interface for every primitive type
(and we can also add more implementations if we need them).

Anyway. Armed with our optional flags, we can make the configuration in
newJaegerTracerFromEnv more sophisticated, giving flags precedence
over environment variables, but also not ignoring environment variables
(which we were previously doing by indiscriminately setting the sampler
type to const).

Signed-off-by: Andrew Mason amason@slack-corp.com

Examples!

(for all examples pwd is ./examples/local)

No flags set, no environment variables set
❯ ./scripts/vtadmin-up.sh
I0527 21:18:46.510285   21494 plugin_jaeger.go:80] Tracing to: localhost:6831 as vtadmin
I0527 21:18:46.510393   21494 plugin_jaeger.go:95] -tracing-sampler-type was not set, and JAEGER_SAMPLER_TYPE was not set, defaulting to const sampler
I0527 21:18:46.510400   21494 plugin_jaeger.go:99] Tracing sampler type const (param: 0.1)
I0527 21:18:46.511120   21494 trace.go:153] successfully started tracing with [opentracing-jaeger]
I0527 21:18:46.512268   21494 server.go:231] server vtadmin listening on :14200
Configure sampler type via environment
❯ JAEGER_SAMPLER_TYPE=probabilistic ./scripts/vtadmin-up.sh 
I0527 22:08:13.579681   26067 plugin_jaeger.go:80] Tracing to: localhost:6831 as vtadmin
I0527 22:08:13.579784   26067 plugin_jaeger.go:99] Tracing sampler type probabilistic (param: 0.1)
I0527 22:08:13.583424   26067 trace.go:153] successfully started tracing with [opentracing-jaeger]
I0527 22:08:13.586123   26067 server.go:231] server vtadmin listening on :14200
Bad sampler type (mostly to check that jaeger would complain, and that we didn't need extra validation)
❯ JAEGER_SAMPLER_TYPE=notatype ./scripts/vtadmin-up.sh
I0527 21:21:28.897821   21552 plugin_jaeger.go:80] Tracing to: localhost:6831 as vtadmin
I0527 21:21:28.897882   21552 plugin_jaeger.go:99] Tracing sampler type notatype (param: 0.1)
E0527 21:21:28.898293   21552 trace.go:147] Unknown sampler type notatype
failed to create a opentracing-jaeger tracer
I0527 21:21:28.900949   21552 server.go:231] server vtadmin listening on :14200
Overriding on the command line

❯ git d -- ./scripts/vtadmin-up.sh | cat
diff --git a/examples/local/scripts/vtadmin-up.sh b/examples/local/scripts/vtadmin-up.sh
index e76a920618..2a1858ec9d 100755
--- a/examples/local/scripts/vtadmin-up.sh
+++ b/examples/local/scripts/vtadmin-up.sh
@@ -10,6 +10,8 @@ vtadmin \
   --http-origin "http://localhost:3000" \
   --http-tablet-url-tmpl "http://{{ .Tablet.Hostname }}:15{{ .Tablet.Alias.Uid }}" \
   --tracer "opentracing-jaeger" \
+  --tracing-sampling-type "probabilistic" \
+  --tracing-sampling-rate 0.2 \
   --grpc-tracing \
   --http-tracing \
   --logtostderr \
❯ JAEGER_SAMPLER_TYPE=rateLimiting ./scripts/vtadmin-up.sh
I0527 22:12:20.122315   26227 plugin_jaeger.go:80] Tracing to: localhost:6831 as vtadmin
I0527 22:12:20.122360   26227 plugin_jaeger.go:99] Tracing sampler type probabilistic (param: 0.2)
I0527 22:12:20.123125   26227 trace.go:153] successfully started tracing with [opentracing-jaeger]
I0527 22:12:20.123967   26227 server.go:231] server vtadmin listening on :14200

Checklist

  • Tests were added or are not required -- N/A
  • Documentation was added or is not required

Deployment Notes

Closes vitessio#8198.

A common pain point with the standard `package flag` is the inability to
tell whether a flag was set on the command line or not. You can check
against the default value, but that's not perfect either, because then
you cannot tell the difference between a flag that was not specified at
all and a flag that was set to exactly the default value.

Enter `OptionalFlag`. We define some structs that wrap a primitive
(`float64` for example) with a boolean flag to track whether `Set()` was
ever called on the flag. Since, for the jaeger sampler params, we only
need a `string` for the sampler type and a `float64` for the sampling
rate, I only implemented `OptionalFlag` for those two types for now.
Plus, all of this becomes obsolete if we move to `cobra`, so I wasn't
highly-motivated to implement this interface for every primitive type
(and we can also add more implementations if we need them).

Anyway. Armed with our optional flags, we can make the configuration in
`newJaegerTracerFromEnv` more sophisticated, giving flags precedence
over environment variables, but also not ignoring environment variables
(which we were previously doing by indiscriminately setting the sampler
type to `const`).

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188 ajm188 added this to In progress in VTAdmin via automation May 28, 2021
@ajm188 ajm188 requested review from systay and deepthi May 28, 2021 02:16
@ajm188 ajm188 merged commit 82c9752 into vitessio:main Jun 4, 2021
VTAdmin automation moved this from In progress to Done Jun 4, 2021
@ajm188 ajm188 deleted the am_jaeger_sampler_configs branch June 4, 2021 02:01
@systay systay added Type: Enhancement Logical improvement (somewhere between a bug and feature) and removed Type: Feature Request labels Jul 6, 2021
ajm188 added a commit to tinyspeck/vitess that referenced this pull request Jul 6, 2021
…figs

[trace] Add optional flags to support flexible jaeger configs

Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188 added a commit to tinyspeck/vitess that referenced this pull request Jul 23, 2021
…figs

[trace] Add optional flags to support flexible jaeger configs

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Build/CI Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
Development

Successfully merging this pull request may close these issues.

[tracing] Support configurable jaeger samplers
3 participants