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.

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

Converge on JAEGER_SAMPLING_ENDPOINT env variable for client's sampling #1849

Closed
3 of 7 tasks
yurishkuro opened this issue Oct 9, 2019 · 7 comments
Closed
3 of 7 tasks
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement meta-issue An tracking issue that requires work in other repos

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Oct 9, 2019

Update 2020-02-07

per discussion #1971 (comment), we settled on the name JAEGER_SAMPLING_ENDPOINT. I'm adding the tracking:

Original ticket

A bunch of tickets and PRs flying about, without a firm agreement on what we want to call this env variable.

Two candidates:

  1. JAEGER_CONFIG_MANAGER_HOST_PORT https://github.com/jaegertracing/jaeger-client-java/pull/554/files#r221994309
  2. JAEGER_CONFIG_ENDPOINT Rename JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLING_ENDPOINT jaeger-client-go#282 (comment)

Drawbacks:

  1. JAEGER_CONFIG_MANAGER_HOST_PORT: Does not allow http vs. https, which may be necessary/useful in some situations
  2. JAEGER_CONFIG_ENDPOINT: Consistent with JAEGER_ENDPOINT for submitting spans, but misleading, because JAEGER_ENDPOINT is the actual URL like http://jaeger-collector:14268/api/traces, but for configuration it's a base url only, to be used with suffix endpoints like /sampling or /baggage
@yurishkuro yurishkuro added meta-issue An tracking issue that requires work in other repos client-libs labels Oct 9, 2019
@yurishkuro
Copy link
Member Author

cc @jpkrohling @pavolloffay

@jpkrohling
Copy link
Contributor

I'm in favor of having a configuration option that accepts a URL, which contains all the components required to derive the actual endpoint value (protocol, host, port, base path).

The other obvious name for the property would be JAEGER_CONFIG_URL, but it has the same drawback as JAEGER_CONFIG_ENDPOINT, as it's not a complete URL (lacks the actual path). JAEGER_CONFIG_BASE_URL would be better in that sense, but I think it's unnecessarily verbose.

@pavolloffay
Copy link
Member

JAEGER_CONFIG_ENDPOINT is fine. First I didn't consider http/https problem.

Do we expect people using different endpoints? If no we could require specifying just the root path and set the base in the implementation to make it easier for users.

@yurishkuro
Copy link
Member Author

Conclusion: go with JAEGER_CONFIG_ENDPOINT, but make it explicit in the description that this is a "base URL".

Agreed?

@yurishkuro
Copy link
Member Author

Update: per discussion #1971 (comment), we settled on the name JAEGER_SAMPLING_ENDPOINT

@yurishkuro yurishkuro changed the title Introduce standard env variable for client config server Introduce standard JAEGER_SAMPLING_ENDPOINT env variable for client's sampling strategies Feb 17, 2020
@yurishkuro yurishkuro changed the title Introduce standard JAEGER_SAMPLING_ENDPOINT env variable for client's sampling strategies Converge on JAEGER_SAMPLING_ENDPOINT env variable for client's sampling Feb 17, 2020
@yurishkuro yurishkuro added good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement labels Feb 17, 2020
@Falco20019
Copy link

C# can be checked as done as it's merged and released as of 0.3.7.

@yurishkuro
Copy link
Member Author

Per #3362, we're sunsetting Jaeger clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement meta-issue An tracking issue that requires work in other repos
Projects
None yet
Development

No branches or pull requests

4 participants