Enable JAEGER_SERVICE_NAME and JAEGER_PROPAGATION env vars to be set … #128
Conversation
…on containers with injected agent Signed-off-by: Gary Brown <[email protected]>
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
+ Coverage 95.99% 96.07% +0.07%
==========================================
Files 27 27
Lines 1148 1171 +23
==========================================
+ Hits 1102 1125 +23
Misses 36 36
Partials 10 10
Continue to review full report at Codecov.
|
|
Ran e2e tests - daemonset still failing but all other tests fine. Want to try it out on a real example before merge - hopefully first thing next week. One point - setting JAEGER_PROPAGATION to |
There was a problem hiding this comment.
Ran e2e tests - daemonset still failing but all other tests fine. Want to try it out on a real example before merge - hopefully first thing next week.
Strange, the e2e test should be passing. Could you confirm that you are able to get a clean run with all tests passing with the latest master?
I'm not sure I understood your question, though. The env vars are still applied to the target application, aren't they? Shouldn't then we always allow both env vars, as we don't know how the application itself is using Jaeger?
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @objectiser)
pkg/inject/sidecar.go, line 22 at r1 (raw file):
const ( serviceName = "JAEGER_SERVICE_NAME"
How about adding a prefix, to make it clear that it's about an env var name, and not the service name itself?
pkg/inject/sidecar.go, line 33 at r1 (raw file):
logrus.Debugf("Skipping sidecar injection for deployment %v", dep.Name) } else { decorate(dep)
+1
pkg/inject/sidecar.go, line 109 at r1 (raw file):
if app, found := dep.Spec.Template.Labels["app"]; found { if len(dep.Namespace) > 0 { app += "." + dep.Namespace
We need to document this. Bonus points if you can add a word or two about why the order is ${name}.${namespace} (DNS-like) and not ${namespace}.${name} (qualifier-like) .
pkg/inject/sidecar_test.go, line 52 at r1 (raw file):
func TestInjectSidecarWithEnvVarsWithNamespace(t *testing.T) { jaeger := v1alpha1.NewJaeger("TestInjectSidecarWithEnvVars")
nit: copy/paste? The instance names are usually following the test name, to avoid one test having an influence on other tests.
pkg/inject/sidecar_test.go, line 77 at r1 (raw file):
assert.Len(t, dep.Spec.Template.Spec.Containers[0].Env, 2) assert.Equal(t, serviceName, dep.Spec.Template.Spec.Containers[0].Env[0].Name) assert.Equal(t, "otherapp", dep.Spec.Template.Spec.Containers[0].Env[0].Value)
We should document this as well: the namespace isn't used at all when app is present.
There was a problem hiding this comment.
Reran and it was clear, so no problem with daemonset test on master. Will rerun on this branch once have done updates.
True - but the environment variables can always be overridden - so was thinking about having sensible defaults. The issue with including both jaeger and b3 propagation formats, is that it increases the number of headers being propagated between services (possibly unnecessarily).
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jpkrohling)
pkg/inject/sidecar.go, line 22 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
How about adding a prefix, to make it clear that it's about an env var name, and not the service name itself?
Done.
pkg/inject/sidecar.go, line 109 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
We need to document this. Bonus points if you can add a word or two about why the order is
${name}.${namespace}(DNS-like) and not${namespace}.${name}(qualifier-like) .
Done.
pkg/inject/sidecar_test.go, line 77 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
We should document this as well: the namespace isn't used at all when
appis present.
Added some comments - but it isn't related to namespace or app - if the deployment includes an explicit env var for JAEGER_SERVICE_NAME, then that value will be used.
Signed-off-by: Gary Brown <[email protected]>
objectiser
changed the title
There was a problem hiding this comment.
I ran the tests against this PR and it was clear:
ok github.com/jaegertracing/jaeger-operator/test/e2e 177.028s
Perhaps your minikube is having some troubles? For the record, here's how I run it:
minikube start --vm-driver kvm2 --cpus 2 --memory 8192
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
pkg/inject/sidecar.go, line 109 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Done.
Sorry, I meant to document this for the end user. This might be too technical for the readme, so, perhaps we just need to open an issue to remember to document it in the future.
Question: do you think we need a flag to disable this feature? The reasoning being that applications might set their service names if there's no env var, which won't be the case with this change if the deployment has the app annotation.
|
Created #130 for the doc issue. How realistic do you think that use case would be? If a service developer took that approach, not sure where they would get the service name from, except a custom config property - in which case, they would probably just use that approach anyway, instead of the Jaeger env vars? |
Good question. Let's record it and see if there's demand for that. |
…on containers with injected agent
Resolves #29
Resolves #74
Signed-off-by: Gary Brown [email protected]