Rename JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_CONFIG_MANAGER_HOST_PORT #325
Conversation
…_PORT Fixes: jaegertracing#282 Signed-off-by: Masroor Hasan <masroor.hasan.n@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #325 +/- ##
==========================================
- Coverage 87.14% 87.06% -0.09%
==========================================
Files 54 54
Lines 2988 2992 +4
==========================================
+ Hits 2604 2605 +1
- Misses 272 274 +2
- Partials 112 113 +1
Continue to review full report at Codecov.
|
Signed-off-by: Masroor Hasan <masroor.hasan.n@gmail.com>
| @@ -72,7 +72,7 @@ type SamplerConfig struct { | |||
| Param float64 `yaml:"param"` | |||
|
|
|||
| // SamplingServerURL is the address of jaeger-agent's HTTP sampling server | |||
| // Can be set by exporting an environment variable named JAEGER_SAMPLER_MANAGER_HOST_PORT | |||
| // Can be set by exporting an environment variable named JAEGER_CONFIG_MANAGER_HOST_PORT | |||
| SamplingServerURL string `yaml:"samplingServerURL"` | |||
There was a problem hiding this comment.
I think this property should be changed as well. Similarly, this should be backwards compatible.
cc @yurishkuro
| } else { | ||
| // deprecated | ||
| if e = os.Getenv(envSamplerManagerHostPort); e != "" { | ||
| jaeger.StdLogger.Infof("deprecated: Please use JAEGER_CONFIG_MANAGER_HOST_PORT") |
There was a problem hiding this comment.
What is deprecated? :) How about something like:
JAEGER_SAMPLER_MANAGER_HOST_PORT was used but is deprecated. Use JAEGER_CONFIG_MANAGER_HOST_PORT instead.
| @@ -116,9 +116,11 @@ func TestNoServiceNameFromEnv(t *testing.T) { | |||
|
|
|||
| func TestSamplerConfigFromEnv(t *testing.T) { | |||
| // prepare | |||
| testMgrHostPort := "http://themaster:80" | |||
There was a problem hiding this comment.
The previous version of the test didn't have the port. Usually, it's better to leave this as it was, to make sure the previous behavior is kept.
| os.Setenv(envSamplerType, "const") | ||
| os.Setenv(envSamplerParam, "1") | ||
| os.Setenv(envSamplerManagerHostPort, "http://themaster") | ||
| os.Setenv(envConfigManagerHostPort, testMgrHostPort) | ||
| os.Setenv(envSamplerManagerHostPort, testMgrHostPort) // deprecated |
There was a problem hiding this comment.
Can you add a new test, where the new option isn't set but the old one is?
| @@ -59,7 +59,8 @@ JAEGER_REPORTER_MAX_QUEUE_SIZE | The reporter's maximum queue size | |||
| JAEGER_REPORTER_FLUSH_INTERVAL | The reporter's flush interval (ms) | |||
| JAEGER_SAMPLER_TYPE | The sampler type | |||
| JAEGER_SAMPLER_PARAM | The sampler parameter (number) | |||
| JAEGER_SAMPLER_MANAGER_HOST_PORT | The host name and port when using the remote controlled sampler | |||
| JAEGER_SAMPLER_MANAGER_HOST_PORT | The host name and port when using the remote controlled sampler (deprecated, use JAEGER_CONFIG_MANAGER_HOST_PORT) | |||
| JAEGER_CONFIG_MANAGER_HOST_PORT | The host name and port when using remote sampling configuration | |||
There was a problem hiding this comment.
According to #326, the variable is actually a URL, not a host:port string. Similar to JAEGER_ENDPOINT, we can call this JAEGER_CONFIG_ENDPOINT.
There was a problem hiding this comment.
Per jaegertracing/jaeger#1849, we want to go with the name JAEGER_SAMPLING_ENDPOINT.
|
Addressed by #511 |
Which problem is this PR solving?
Resolves #282
Short description of the changes
Renames the environment variable
JAEGER_SAMPLER_MANAGER_HOST_PORTtoJAEGER_CONFIG_MANAGER_HOST_PORTto denote the endpoint is not just for setting sampling server but rather is the endpoint for remote sampling configuration.Signed-off-by: Masroor Hasan masroor.hasan.n@gmail.com