Passing default sampling strategies file as environment variable

[Ashmita152]

Signed-off-by: Ashmita Bohara [email protected]

Which problem is this PR solving?

Closes #3022

Short description of the changes

Setting env var SAMPLING_STRATEGIES_FILE with default value and removing default argument in all-in-one's Dockerfile.

[yurishkuro]

please post some testing evidence

[codecov]

Codecov Report

Merging #3027 (031c3c2) into master (63bf567) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3027   +/-   ##
=======================================
  Coverage   95.99%   95.99%           
=======================================
  Files         224      224           
  Lines        9773     9773           
=======================================
  Hits         9382     9382           
- Misses        322      323    +1     
+ Partials       69       68    -1     

Impacted Files	Coverage Δ
model/converter/thrift/jaeger/from_domain.go	100.00% <ø> (ø)
...lugin/sampling/strategystore/adaptive/processor.go	99.07% <0.00%> (-0.93%)	⬇️
plugin/storage/integration/integration.go	77.34% <0.00%> (-0.56%)	⬇️
cmd/query/app/static_handler.go	96.77% <0.00%> (ø)
plugin/storage/badger/spanstore/reader.go	96.08% <0.00%> (+0.71%)	⬆️
cmd/collector/app/server/zipkin.go	76.92% <0.00%> (+3.84%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0d0518...031c3c2. Read the comment docs.

[Ashmita152]

Hi @yurishkuro

Thank you for the review. To test this change, I tried the following:

1. Built an image from my branch locally:

❯ git branch
* cmd-to-env-sampling
  master
❯ make build-all-in-one GOOS=linux GOARCH=amd64
<redacted>
❯ docker build -q -f cmd/all-in-one/Dockerfile --target release --tag ashmita/all-in-one:latest cmd/all-in-one --build-arg base_image=localhost/baseimg:1.0.0-alpine-3.12 --build-arg debug_image=localhost/debugimg:1.0.0-golang-1.15-alpine --build-arg TARGETARCH=amd64
sha256:7d4515527ffed5618eeccf1bbb94f01a9ca624d755f76cabbf7c77671fef481dt

2. Started the image locally:

❯ docker run --name=jaeger-all-in-one ashmita/all-in-one --log-level=error
2021/05/24 14:17:27 maxprocs: Leaving GOMAXPROCS=6: CPU quota undefined

3. Exec into the container and check the environment variables:

❯ docker exec -it jaeger-all-in-one /bin/sh
/ # env | grep SAMPLING_STRATEGIES_FILE
SAMPLING_STRATEGIES_FILE=/etc/jaeger/sampling_strategies.json

Having this environment variable didn't give me confidence that this file is being read.

4. I added a log line in the code where the strategies file being read:

❯ git diff
diff --git a/plugin/sampling/strategystore/static/strategy_store.go b/plugin/sampling/strategystore/static/strategy_store.go
index 704c9453..046814de 100644
--- a/plugin/sampling/strategystore/static/strategy_store.go
+++ b/plugin/sampling/strategystore/static/strategy_store.go
@@ -137,6 +137,8 @@ func samplingStrategyLoader(strategiesFile string) strategyLoader {
                currBytes, err := ioutil.ReadFile(filepath.Clean(strategiesFile))
                if err != nil {
                        return nil, fmt.Errorf("failed to read strategies file %s: %w", strategiesFile, err)
+               } else {
+                       fmt.Printf("successfully read strategies file: %s\n", strategiesFile)
                }
                return currBytes, nil
        }

5. Again built and ran the container:

❯ docker run --name=jaeger-all-in-one ashmita/all-in-one --log-level=error
2021/05/24 14:17:27 maxprocs: Leaving GOMAXPROCS=6: CPU quota undefined
successfully read strategies file: /etc/jaeger/sampling_strategies.json

Not fully sure that I am testing it in the right way.

Regards.

[yurishkuro]

Thanks! A couple of suggestions:

1. run a curl command against the sampling endpoint with some made-up service name to ensure you're getting back the default sampling probability (you may want to build an image with a sampling file that has a recognizable default probability like 0.1234)

$ curl "http:/localhost:5778/sampling?service=foobar"
{"strategyType":"PROBABILISTIC","probabilisticSampling":{"samplingRate":0.001}}

2. run the image with an overwritten env var pointing to a local sampling file with different probability and confirm it with a curl (remember to mount the file into container to make it visible from inside)

[Ashmita152]

Hi @yurishkuro

Thank you for feedbacks. I tried the steps you mentioned.

1. First built the image from this PR branch

❯ docker run -d  --name=jaeger-all-in-one ashmita/all-in-one
❯ docker exec -it jaeger-all-in-one /bin/sh
/ # curl "http:/localhost:5778/sampling?service=foobar"
{"strategyType":"PROBABILISTIC","probabilisticSampling":{"samplingRate":1}}/

2. Modified the probability in the default file in the repo and rebuilt the image

❯ git diff
diff --git a/cmd/all-in-one/sampling_strategies.json b/cmd/all-in-one/sampling_strategies.json
index cbad0834..99a3cf5b 100644
--- a/cmd/all-in-one/sampling_strategies.json
+++ b/cmd/all-in-one/sampling_strategies.json
@@ -1,6 +1,6 @@
 {
   "default_strategy": {
     "type": "probabilistic",
-    "param": 1
+    "param": 0.1234
   }
 }
❯ docker run -d  --name=jaeger-all-in-one ashmita/all-in-one
❯ docker exec -it jaeger-all-in-one /bin/sh
/ # curl "http:/localhost:5778/sampling?service=foobar"
{"strategyType":"PROBABILISTIC","probabilisticSampling":{"samplingRate":0.1234}}/ #

3. Added a different strategies file.

❯ cat /tmp/sampling_strategies.json
{
  "default_strategy": {
    "type": "probabilistic",
    "param": 0.4321
  }
}
❯ docker run -d -e SAMPLING_STRATEGIES_FILE=/tmp/sampling_strategies.json -v /tmp/sampling_strategies.json:/tmp/sampling_strategies.json --name=jaeger-all-in-one ashmita/all-in-one
❯ docker exec -it jaeger-all-in-one /bin/sh
/ # curl "http:/localhost:5778/sampling?service=foobar"
{"strategyType":"PROBABILISTIC","probabilisticSampling":{"samplingRate":0.4321}}/ #

Regards.

[yurishkuro]

Great, thanks!