Update default samplingServerURL to include /sampling path

[JorritSalverda]

Resolves #157

Signed-off-by: Jorrit Salverda jorrit.salverda@gmail.com

Which problem is this PR solving?

The default sampling server url misses the /sampling path, which leads to an incorrect response and inability to use remote configuration.

Short description of the changes

Updated the default url and added a test.

[JorritSalverda]

I have no experience writing C++, so let me know if I do something unusual for the language. Although there's hardly any coding involved :)

[yurishkuro]

Have you tested it against Jaeger server? I don't think the crossdock integration test in this repo actually tests polling for sampling strategies, it only tests spans submission. I believe there is a difference between / and /sampling endpoints in the returned data format.

[hiruna]

hi can we get this merged ?

[yurishkuro]

I would like to see some proof of testing with the agent.

[hiruna]

@yurishkuro , there is a difference between / and /sampling (correct me if im wrong), / returns the default value for a remote sampler, which is probabilistic | 0.5 this was mentioned in #157

[yurishkuro]

The difference is not in the semantics of the response but in the format of the data, how the thrift enums were encoded into json (numbers instead of strings).

[skwiffhub]

@yurishkuro Is this fix going to be merged at some point? During our debugging using the 0.5.0 jaeger-client-cpp, we observe exactly what @JorritSalverda described in #157:

Using samplingServerURL of http://127.0.0.1:5778 results in an HTTP response from the sampling server containing this JSON:

{"strategyType":0,"probabilisticSampling":{"samplingRate":0.001}}

which results in the client hitting the exception at line 1352 of include/nlohmann/json.hpp (v3.6.1):

        JSON_THROW(type_error::create(302, "type must be string, but is " + std::string(j.type_name())));

Whereas using samplingServerURL of http://127.0.0.1:5778/sampling results in:

{"strategyType":"PROBABILISTIC","probabilisticSampling":{"samplingRate":0.001}}

No exception occurs and the JSON is parsed and the sampling rate updated properly.

Based on the RemoteSamplingJSON.h implementation, it appears to only support strategyType of "PROBABILISTIC".

[yurishkuro]

@skwiffhub are you able to test with the fix in this PR and confirm that it works?

[AppVeyorBot]

✅ Build jaeger-client-cpp 31 completed (commit 24ba8de49e by @yurishkuro)

[skwiffhub]

@yurishkuro Yes, thanks, we were able to test with the fix. We did not verify the ConfigTest.cpp portion of the commit though.

One other thing to check for is sample instances of config.yml files that contain samplingServerURL and make sure they are updated to reflect what is in the current source code.

[yurishkuro]

@skwiffhub thanks for the confirmation, I will merge it. I am not too concerned with config test because it actually has a unit test, but there's no integration test with the real agent.

[yurishkuro]

Thanks for the PR, @JorritSalverda

[hiruna]

Is this going to be released soon ? @yurishkuro

[hiruna]

any update on this @yurishkuro @JorritSalverda