Make all-in-one.yaml file independant of sampling-strategies.json file #6431

adityachopra29 · 2024-12-27T19:35:49Z

Which problem is this PR solving?

fix [bug]: v2 all-in-one binary cannot find sampling strategies file #6338

Description of the changes

Removed the hardcoded path in the default configuration (all-in-one.yaml).
updated tests accordingly

How was this change tested?

Since some of the tests are using hardcoded services in sampling-strategies.json (the service "foo").

jaeger/cmd/jaeger/internal/extension/remotesampling/extension_test.go

Line 83 in b02900c

the sampling-strategies.json file could not be removed
Also, right now there are some tests, namely:
1.) TestServerHTTP_TracesRequest,
2.) all 3 tests in cmd/query/app
which are not passing even on the main branch itself (according to me, otherwise I have made some mistake), and hence these same tests are not passing after I made my changes (my changes should not affect them anyway).

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

cmd/jaeger/internal/extension/remotesampling/config_test.go

yurishkuro

you are working off a stale main, please rebase. There should be no changes to submodules in your PR
you have not made any changes to the logic, so why are you sending this for review?
you need to test that sampling endpoint returns expected stratery

$ curl "http://localhost:5778/?service=x"
{"strategyType":0,"probabilisticSampling":{"samplingRate":1}}%

cmd/jaeger/internal/extension/remotesampling/extension_test.go

adityachopra29 · 2024-12-30T11:50:11Z

Hi @yurishkuro,
Kindly review the PR now.

the output is coming correct testing the endpoint

$ curl "http://localhost:5778/?service=x"
{"strategyType":0,"probabilisticSampling":{"samplingRate":0.001}}%

I have updated the base main branch
no other changes were suggested in logic, hence rest code is same

cmd/jaeger/internal/extension/remotesampling/config_test.go

codecov · 2024-12-30T15:51:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (fc518ce) to head (0b94890).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6431      +/-   ##
==========================================
- Coverage   96.27%   96.25%   -0.03%     
==========================================
  Files         369      369              
  Lines       21048    21048              
==========================================
- Hits        20264    20259       -5     
- Misses        600      604       +4     
- Partials      184      185       +1

Flag	Coverage Δ
badger_v1	`10.53% <ø> (ø)`
badger_v2	`2.59% <ø> (ø)`
cassandra-4.x-v1-manual	`16.41% <ø> (ø)`
cassandra-4.x-v2-auto	`2.52% <ø> (ø)`
cassandra-4.x-v2-manual	`2.52% <ø> (ø)`
cassandra-5.x-v1-manual	`16.41% <ø> (ø)`
cassandra-5.x-v2-auto	`2.52% <ø> (ø)`
cassandra-5.x-v2-manual	`2.52% <ø> (ø)`
elasticsearch-6.x-v1	`20.14% <ø> (+<0.01%)`	⬆️
elasticsearch-7.x-v1	`20.22% <ø> (ø)`
elasticsearch-8.x-v1	`20.37% <ø> (ø)`
elasticsearch-8.x-v2	`2.58% <ø> (ø)`
grpc_v1	`12.18% <ø> (-0.01%)`	⬇️
grpc_v2	`8.96% <ø> (ø)`
kafka-3.x-v1	`10.37% <ø> (ø)`
kafka-3.x-v2	`2.59% <ø> (ø)`
memory_v2	`2.59% <ø> (ø)`
opensearch-1.x-v1	`20.26% <ø> (ø)`
opensearch-2.x-v1	`20.25% <ø> (-0.01%)`	⬇️
opensearch-2.x-v2	`2.59% <ø> (+<0.01%)`	⬆️
tailsampling-processor	`0.39% <ø> (ø)`
unittests	`95.11% <ø> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

## Which problem is this PR solving? - Accidentally stumbled upon this when looking at #6431 - Turns out v2 config did not support all the options from v1 ## Description of the changes - Add support for reload interval ## How was this change tested? `go run ./cmd/jaeger` ``` $ curl "http://localhost:5778/?service=x" {"strategyType":0,"probabilisticSampling":{"samplingRate":1}}% ``` Edit cmd/jaeger/sampling-strategies.json to change default to 0.1. Observe server logs showing new values. ``` $ curl "http://localhost:5778/?service=x" {"strategyType":0,"probabilisticSampling":{"samplingRate":0.1}}% ``` --------- Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2024-12-30T17:03:06Z

$ docker run -p 5778:5778 jaegertracing/jaeger:2.1.0

$ curl "http://localhost:5778/?service=x"
{"strategyType":0,"probabilisticSampling":{"samplingRate":1}}%

The current default behavior is to return probability 1.0, not 0.001.

There is a hardcoded constant defaultSamplingProbability = 0.001, but we don't want to change it because jaeger-v1 should still be using that, so we need to make that value configurable in the static/provider

yurishkuro · 2024-12-30T17:04:16Z

make sure to pull from remote branch before you continue work, to pick up the changes I made

adityachopra29 · 2024-12-30T19:51:37Z

Okay. So the initial issue regarding the dependancy of sampling-strategies.json file in all-in-one.yaml is resolved right? This is a new issue which needs to be solved?
To make SamplingRate configurable in static/provider, keeping defaultSamplingProbability const?
If yes, then I plan on implementing this by creating a factor (with default value 1000), which I will multiply defaultSamplingProbability with when returning SamplingRate. Am I on the right track?

yurishkuro · 2024-12-30T20:42:47Z

The original issue is not resolved since these changes return different results from what v2.1.0 Jaeger container returns.

There is no need for any multiplier, the default probability should be made parameterizable via the provider API so that we can pass it from jaeger-v2 sampling extension (see #6441 for the pattern).

Signed-off-by: adityachopra29 <[email protected]>

Signed-off-by: Yuri Shkuro <[email protected]>

…s for the same.

adityachopra29 · 2025-01-05T12:22:17Z

@yurishkuro Please have a look once now.

A parameter for DefaultSamplingProbability has been created which can be filled using a flag, or using the yaml file input default_sampling_probability.
A flag called "sampling.stategies-default-sampling-probability" has been created for the same
If no input is given for DefaultSamplingProbability, it defaults to "defaultDefaultSamplingProbability" = 0.001. (which was originally used)
DefaultSamplingProbability has been given value 0.001 in all-in-one.yaml (to give the default value as before)
In all the tests given in the provider_test.go file, initially they used defaultSamplingProbability = 0.001. Since now this parameterized, I have used the value of defaultDefaultSamplingProbability = 0.001 instead now to keep the default behavior.
Since no changes have been made to the original defaultSamplingProbability variable, the output still comes as follows:

 curl "http://localhost:5778/?service=x"
{"strategyType":0,"probabilisticSampling":{"samplingRate":0.001}}%

Instead of

{"strategyType":0,"probabilisticSampling":{"samplingRate":1}}%

whch you wanted. I wanted to confirm what change I have to do to set this.

yurishkuro

please remove idl submodule change

yurishkuro · 2025-01-05T15:53:04Z

cmd/jaeger/internal/extension/remotesampling/config.go

@@ -104,6 +107,10 @@ func (cfg *Config) Validate() error {
 		return errNegativeInterval
 	}

+	if cfg.File != nil && (cfg.File.DefaultSamplingProbability > 1 || cfg.File.DefaultSamplingProbability < 0) {


I think you should be able to achieve similar validation by adding a valid tag to the field, perhaps valid:range(0, 1)

yurishkuro · 2025-01-05T15:54:29Z

cmd/jaeger/internal/extension/remotesampling/config.go

 	// ReloadInterval is the time interval to check and reload sampling strategies file
 	ReloadInterval time.Duration `mapstructure:"reload_interval"`
+	// DefaultSamplingProbability is the sampling probability used by the Strategy Store for static sampling
+	DefaultSamplingProbability float64 `mapstructure:"default_sampling_probability"`


you need to assign default value to this in factory/createDefaultConfig

yurishkuro · 2025-01-05T15:55:00Z

cmd/jaeger/internal/all-in-one.yaml

      reload_interval: 1s
+      default_sampling_probability: 0.001


why not 1.0? I think this would be equivalent to the previous config where the stategies file was specified

yurishkuro

on the right track

yurishkuro · 2025-01-05T15:56:58Z

plugin/sampling/strategyprovider/static/options.go

+	samplingStrategiesFile                       = "sampling.strategies-file"
+	samplingStrategiesReloadInterval             = "sampling.strategies-reload-interval"
+	samplingStrategiesBugfix5270                 = "sampling.strategies.bugfix-5270"
+	samplingStrategiesDefaultSamplingProbability = "sampling.stategies-default-sampling-probability"


Suggested change

samplingStrategiesDefaultSamplingProbability = "sampling.stategies-default-sampling-probability"

samplingStrategiesDefaultSamplingProbability = "sampling.default-sampling-probability"

yurishkuro · 2025-01-05T15:57:32Z

plugin/sampling/strategyprovider/static/options.go

 }

 // AddFlags adds flags for Options
 func AddFlags(flagSet *flag.FlagSet) {
 	flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no reloading")
 	flagSet.String(samplingStrategiesFile, "", "The path for the sampling strategies file in JSON format. See sampling documentation to see format of the file")
 	flagSet.Bool(samplingStrategiesBugfix5270, true, "Include default operation level strategies for Ratesampling type service level strategy. Cf. https://github.com/jaegertracing/jaeger/issues/5270")
+	flagSet.Float64(samplingStrategiesDefaultSamplingProbability, defaultDefaultSamplingProbability, "Sampling probability used by the Strategy Store for static sampling. Value must be between 0 and 1.")


defaultDefaultSamplingProbability

default-default?

yes. it is the default value for options.DefaultSamplingProbability ... so i named it accordingly
And wherever defaultSamlingProbability was used I replaced it with this.

yurishkuro · 2025-01-05T15:59:48Z

plugin/sampling/strategyprovider/static/provider.go

 	}

 	if options.ReloadInterval > 0 {
-		go h.autoUpdateStrategies(ctx, options.ReloadInterval, loadFn)
+		go h.autoUpdateStrategies(ctx, options.ReloadInterval, loadFn, options)


if you are passing options no need to pass options.ReloadInterval

do we need to pass either of those? It seems h already has access to h.options

yes it seems we dont need to pass them. Removed now.

## Which problem is this PR solving? - Accidentally stumbled upon this when looking at jaegertracing#6431 - Turns out v2 config did not support all the options from v1 ## Description of the changes - Add support for reload interval ## How was this change tested? `go run ./cmd/jaeger` ``` $ curl "http://localhost:5778/?service=x" {"strategyType":0,"probabilisticSampling":{"samplingRate":1}}% ``` Edit cmd/jaeger/sampling-strategies.json to change default to 0.1. Observe server logs showing new values. ``` $ curl "http://localhost:5778/?service=x" {"strategyType":0,"probabilisticSampling":{"samplingRate":0.1}}% ``` --------- Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]>

…ptions' input from functions

adityachopra29 · 2025-01-07T19:17:09Z

@yurishkuro I have made the changes. Please have a look.
Also, I found that defaultSamplingProbability was not being used anywhere, except in the provider tests(which I replaced with DefaultDefaultSamplingProbability,as you can see. So I removed it.

adityachopra29 requested a review from a team as a code owner December 27, 2024 19:35

adityachopra29 requested a review from pavolloffay December 27, 2024 19:35

dosubot bot added the area/sampling label Dec 27, 2024

yurishkuro reviewed Dec 27, 2024

View reviewed changes

cmd/jaeger/internal/extension/remotesampling/config_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Dec 27, 2024

View reviewed changes

cmd/jaeger/internal/extension/remotesampling/extension_test.go Outdated Show resolved Hide resolved

adityachopra29 closed this Dec 27, 2024

adityachopra29 reopened this Dec 30, 2024

adityachopra29 force-pushed the fix-sampling-config branch from d95c1a3 to a884a32 Compare December 30, 2024 11:41

yurishkuro reviewed Dec 30, 2024

View reviewed changes

cmd/jaeger/internal/extension/remotesampling/config_test.go Show resolved Hide resolved

yurishkuro mentioned this pull request Dec 30, 2024

Support sampling file reload interval #6440

Merged

yurishkuro added the changelog:bugfix-or-minor-feature label Dec 30, 2024

yurishkuro mentioned this pull request Dec 30, 2024

Fix: Remove hardcoded sampling-strategies.json dependency in Jaeger in-one binary #6356

Closed

4 tasks

adityachopra29 and others added 8 commits January 5, 2025 16:18

Make all-in-one.yaml file independant of sampling-strategies.json file

ee06229

Signed-off-by: adityachopra29 <[email protected]>

add the errNoProvider test back again

acd5500

Signed-off-by: adityachopra29 <[email protected]>

replace null with blank for sampling-strategy in all-in-one.yaml

b57c01b

Signed-off-by: adityachopra29 <[email protected]>

Added back test for checking non-null provider

69ef912

Signed-off-by: adityachopra29 <[email protected]>

set back the path to sampling strategies file in exetension test

9759455

Signed-off-by: adityachopra29 <[email protected]>

restore idl

584db5c

Signed-off-by: Yuri Shkuro <[email protected]>

restore-test

095434e

Signed-off-by: Yuri Shkuro <[email protected]>

make DefaultSamplingProbability parameterizable using flag

8097990

adityachopra29 force-pushed the fix-sampling-config branch from bb1e6b0 to 8097990 Compare January 5, 2025 10:59

adityachopra29 added 2 commits January 5, 2025 16:52

remove 'required' tag from Path variable in FileConfig

d0a0d18

create default_sampling_probability config for file in yaml, add test…

bde692c

…s for the same.

yurishkuro reviewed Jan 5, 2025

View reviewed changes

adityachopra29 and others added 2 commits January 8, 2025 00:37

Merge branch 'jaegertracing:main' into fix-sampling-config

d1a070a

use 'valid' tag for DefaultSamplingProbability, remove unnecessary 'o…

c0d6ee2

…ptions' input from functions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make all-in-one.yaml file independant of sampling-strategies.json file #6431

Make all-in-one.yaml file independant of sampling-strategies.json file #6431

adityachopra29 commented Dec 27, 2024

yurishkuro left a comment

adityachopra29 commented Dec 30, 2024

codecov bot commented Dec 30, 2024 •

edited

Loading

yurishkuro commented Dec 30, 2024

yurishkuro commented Dec 30, 2024

adityachopra29 commented Dec 30, 2024 •

edited

Loading

yurishkuro commented Dec 30, 2024 •

edited

Loading

adityachopra29 commented Jan 5, 2025

yurishkuro left a comment

yurishkuro Jan 5, 2025

yurishkuro Jan 5, 2025

yurishkuro Jan 5, 2025

yurishkuro left a comment

yurishkuro Jan 5, 2025

yurishkuro Jan 5, 2025

adityachopra29 Jan 7, 2025

yurishkuro Jan 5, 2025

adityachopra29 Jan 7, 2025

adityachopra29 commented Jan 7, 2025

	samplingStrategiesDefaultSamplingProbability = "sampling.stategies-default-sampling-probability"
	samplingStrategiesDefaultSamplingProbability = "sampling.default-sampling-probability"

Make all-in-one.yaml file independant of sampling-strategies.json file #6431

Are you sure you want to change the base?

Make all-in-one.yaml file independant of sampling-strategies.json file #6431

Conversation

adityachopra29 commented Dec 27, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

yurishkuro left a comment

Choose a reason for hiding this comment

adityachopra29 commented Dec 30, 2024

codecov bot commented Dec 30, 2024 • edited Loading

Codecov Report

yurishkuro commented Dec 30, 2024

yurishkuro commented Dec 30, 2024

adityachopra29 commented Dec 30, 2024 • edited Loading

yurishkuro commented Dec 30, 2024 • edited Loading

adityachopra29 commented Jan 5, 2025

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityachopra29 commented Jan 7, 2025

codecov bot commented Dec 30, 2024 •

edited

Loading

adityachopra29 commented Dec 30, 2024 •

edited

Loading

yurishkuro commented Dec 30, 2024 •

edited

Loading