All-in-one docker image: SAMPLING_STRATEGIES_FILE env var can't be used (without args overwrite)

[calohmn]

Describe the bug
I'm using the all-in-one docker image in a Kubernetes deployment and want to set a sampling-strategies file.
The obvious thing (to me) seems to be setting the SAMPLING_STRATEGIES_FILE env var in the K8s deployment descriptor.
But that doesn't work - still the default /etc/jaeger/sampling_strategies.json file gets used. And that is not even obvious as there isn't a log output giving info about which file is used (see #2197).

Only a look at the all-in-one dockerfile

jaeger/cmd/all-in-one/Dockerfile

Line 67 in c991a00

CMD ["--sampling.strategies-file=/etc/jaeger/sampling_strategies.json"]

reveals that the sampling.strategies-file param is set there - obviously having precedence.
So, I would either need to set args: [""] (in the K8s deployment description spec.template.spec.containers section) in order to be able to use the SAMPLING_STRATEGIES_FILE env var.
Or, I would have to use args: ["--sampling.strategies-file=/path/my_sampling_strategies.json"]. But that means I have to check the all-in-one dockerfile to see what other parameters I'm possibly overwriting with this.

Suggested fix
To me it seems that things would be easier, more intuitive and less error-prone if the all-in-one image docker file would set a SAMPLING_STRATEGIES_FILE env var with the default sampling-strategies file instead of using the parameter in the CMD entry.

Then a SAMPLING_STRATEGIES_FILE env var in the K8s deployment descriptor can override the default. Alternatively, using args: ["--sampling.strategies-file=/path/my_sampling_strategies.json"] in the K8s deployment descriptor instead should still work, the command line parameter having precedence then.

To Reproduce
Steps to reproduce the behavior:

1. Create a K8s deployment with the all-in-one image, defining a none-existing strategies file via env var, which should cause an error on startup

 env:
  - name: SAMPLING_STRATEGIES_FILE
    value: "none-existing-sampling-strategies.json"

2. See that there is no error on pod startup, meaning that setting SAMPLING_STRATEGIES_FILE has had no effect.
3. add args: [""] on the same level as env and update deployment to see pod startup failure - meaning this time the env var got actually used.

Expected behavior
SAMPLING_STRATEGIES_FILE env var can be used in the K8s deployment description without further required changes.

Version (please complete the following information):

• OS: Linux
• Jaeger version: 1.22
• Deployment: Kubernetes

[yurishkuro]

+1, would you like to submit a fix?

[jpkrohling]

So, I would either need to set args: [""] (in the K8s deployment description spec.template.spec.containers section) in order to be able to use the SAMPLING_STRATEGIES_FILE env var.
Or, I would have to use args: ["--sampling.strategies-file=/path/my_sampling_strategies.json"]. But that means I have to check the all-in-one dockerfile to see what other parameters I'm possibly overwriting with this.

Or you could mount your strategies file in the place that is spefied in the CMD?

To me it seems that things would be easier, more intuitive and less error-prone if the all-in-one image docker file would set a SAMPLING_STRATEGIES_FILE env var with the default sampling-strategies file instead of using the parameter in the CMD entry.

Sounds good to me.

Then a SAMPLING_STRATEGIES_FILE env var in the K8s deployment descriptor can override the default. Alternatively, using args: ["--sampling.strategies-file=/path/my_sampling_strategies.json"] in the K8s deployment descriptor instead should still work, the command line parameter having precedence then.

I think we don't actually need this setting/file in the container to begin with... Perhaps it should just be removed from the Docker image?

[sophokles73]

I think we don't actually need this setting/file in the container to begin with... Perhaps it should just be removed from the Docker image?

+1

[yurishkuro]

$ m cmd/all-in-one/sampling_strategies.json
{
  "default_strategy": {
    "type": "probabilistic",
    "param": 1
  }
}

The file tells all SDKs connected to all-in-one to use 100% sampling. There's no other way afaik to configure this on the backend. I assume without this file the default would be something like 0.001 (we could introduce a param to control that value without a file)

[jpkrohling]

Is this also the default only for all-in-one?

[sophokles73]

Or simply keep the file in the image but make its usage optional, i.e. do not use the --sampling.strategies-file parameter by default but let the user decide to set it (or not)

[yurishkuro]

It's the default for the SDKs using remote sampler.

all-in-one itself is internally traced with 100% (can be overridden via env)

func initTracer(metricsFactory metrics.Factory, logger *zap.Logger) io.Closer {
        traceCfg := &jaegerClientConfig.Configuration{
                ServiceName: "jaeger-query",
                Sampler: &jaegerClientConfig.SamplerConfig{
                        Type:  "const",
                        Param: 1.0,
                },
                RPCMetrics: true,
        }

[jpkrohling]

Sorry, I meant to ask whether remote samplers getting the strategy from an all-in-one would get the same strategy as from a jaeger-collector. Are they both serving the same strategies? If so, shouldn't we be changing the default to 1 instead?

[yurishkuro]

yes, it's the same remote sampling mechanism.

shouldn't we be changing the default to 1 instead?

I think that would be a dangerous breaking change.

[calohmn]

So this means the strategies file is needed for now, and we can stick to defining the env var in the Dockerfile, instead of using the CMD parameter, correct?

ENV SAMPLING_STRATEGIES_FILE=/etc/jaeger/sampling_strategies.json

I would gladly provide a PR but since getting company approval for contributing here would take a while and since it's such a small change, I would appreciate it if someone else could do it.