Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replicate /sampling endpoint from agent in the collector #1971

Closed
yurishkuro opened this issue Dec 10, 2019 · 9 comments · Fixed by #1990
Closed

Replicate /sampling endpoint from agent in the collector #1971

yurishkuro opened this issue Dec 10, 2019 · 9 comments · Fixed by #1990
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

Requirement - what kind of business use case are you trying to solve?

When using HTTP reporters in the clients to export tracing data directly to the collectors, users still want to be able to use remote sampler that loads centrally managed sampling strategies.

Problem - what in Jaeger blocks you from solving the requirement?

The /sampling endpoint is only implemented in the agent today.

Proposal - what do you suggest to solve the problem or improve the existing situation?

Expose the same endpoint from the collectors, e.g. /api/sampling.
@yurishkuro yurishkuro added changelog:new-feature Change that should be called out as new feature in CHANGELOG good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement labels Dec 10, 2019
@yurishkuro
Copy link
Member Author

When this is done, update FAQ docs, cf. jaegertracing/documentation#335

@namratachaudhary
Copy link

Hi
If this is still open, I can take it up.

@jpkrohling
Copy link
Contributor

It's yours.

@RickyRajinder RickyRajinder mentioned this issue Dec 21, 2019
Merged
@RickyRajinder
Copy link
Contributor

Hi @namratachaudhary I have already submitted a PR for this issue.

@yurishkuro yurishkuro mentioned this issue Dec 31, 2019
Merged
@yurishkuro
Copy link
Member Author

Open question: can clients be configured with a different sampling URL?

@RickyRajinder RickyRajinder mentioned this issue Jan 1, 2020
Merged
@yurishkuro
Copy link
Member Author

yurishkuro commented Jan 1, 2020 •
edited
Loading

Clients support the following config options:

  • JAEGER_ENDPOINT, e.g. http(s)://collector:port/api/traces
  • JAEGER_SAMPLER_MANAGER_HOST_PORT - name implies no path is allowed

Possible solutions:

  • We could introduce JAEGER_SAMPLING_ENDPOINT, but that still leaves out other configurations like baggage restrictions.
  • We could introduce JAEGER_CONFIG_ENDPOINT, but that wouldn't be a real endpoint, but a base address, with /sampling or /baggage path added by the client as needed.
  • We could introduce JAEGER_API (or something along these lines) that will be the base address, e.g. http(s)://collector:port/api used by both span reporter (by adding /traces) and sampler (by adding /sampling).

Q: is it better to hardcode the final segments of the URL in the clients or make them configurable? Strictly speaking the URL path is part of the contract, e.g. if we introduce a different format of the sampling output it should become /api/v2/sampling or something like that.

cc @jpkrohling @pavolloffay @objectiser

@pavolloffay
Copy link
Member

Q: is it better to hardcode the final segments of the URL in the clients or make them configurable?

I would lean towards the consistency and require configuring the full URL. It's more flexible and one could deploy it on different path.

@yurishkuro
Copy link
Member Author

@pavolloffay so you'd vote for JAEGER_SAMPLING_ENDPOINT?

@yurishkuro yurishkuro mentioned this issue Jan 28, 2020
Merged
ecourreges-orange added a commit to ecourreges-orange/jaeger-client-cpp that referenced this issue Jan 28, 2020
@ecourreges-orange
rename JAEGER_SAMPLER_MANAGER_HOST_PORT into JAEGER_SAMPLING_ENDPOINT…
2d93d85 
… as per jaegertracing/jaeger#1971 (comment)

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>
yurishkuro pushed a commit to jaegertracing/jaeger-client-cpp that referenced this issue Jan 29, 2020
Fix loading of sampling endpoint address from JAEGER_SAMPLING_ENDPOIN…
9e1b39c 
…T environment variable (#200)

* fix configuration from environment variables

fix conversion to double of JAEGER_SAMPLER_PARAM
rename env var JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLER_SERVER_URL
properly load JAEGER_SAMPLER_SERVER_URL

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>

* proper unit testing of RemotelyControlledSampler

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>

* include /sampling in all samplingServerURL examples to avoid confusion
/ or /whatever does not work for this driver: the agent answers slightly differently with a numerical strategyType

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>

* rename JAEGER_SAMPLER_MANAGER_HOST_PORT into JAEGER_SAMPLING_ENDPOINT as per jaegertracing/jaeger#1971 (comment)

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>
@yurishkuro yurishkuro mentioned this issue Feb 7, 2020
Closed
7 tasks
@Falco20019 Falco20019 mentioned this issue Feb 7, 2020
Closed
@yurishkuro
Copy link
Member Author

This is done.

This was referenced Oct 28, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add /api/sampling endpoint in collector RickyRajinder/jaeger
5 participants
@jpkrohling @yurishkuro @pavolloffay @namratachaudhary @RickyRajinder