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

Add environment variable for third party AWS X-Ray Sampler #1829

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

anuraaga
Copy link
Contributor

Changes

Adds an environment variable value for the AWS X-Ray Sampler. Being a vendor sampler, it doesn't make much sense to define the Sampler itself in the spec, this is only to make sure language-specific SDK extensions offer a consistent experience for environment variables, similar to the AWS propagator.

@yurishkuro
Copy link
Member

I don't think vendor codes like that belong to the spec. There was a proposal for "custom" samplers here: https://github.com/open-telemetry/opentelemetry-specification/pull/1791/files#r674910699

@anuraaga
Copy link
Contributor Author

I thought there was a goal to unify the behavior among Otel sdks in terms of configuring vendor behavior too isn't there? Otherwise different languages can end up with inconsistent behavior

@yurishkuro
Copy link
Member

to unify the behavior among Otel sdks in terms of configuring vendor behavior

Yes, we should have that, but I would prefer to do it without the actual vendor codes in the spec. For example, the definition of OTEL_TRACES_SAMPLER could say that it allows values from the existing list PLUS vendor-specific names, but to use those vendor names there must be a specific documented mechanism in the SDK to register the factory for the vendor-specific sampler. This way the specs defines what the SDKs need to do/provide without having to list any vendor-codes.

The problem with listing a vendor code is that it raises the question - what is the SDK supposed to do when it sees one? To answer that, we need the factory mechanism above, but once we have the mechanism we don't need the specific codes in the spec.

@tedsuo
Copy link
Contributor

tedsuo commented Jul 27, 2021

I think this may be a slightly different issue. What @anuraaga wants, I believe, is for samplers to be loaded as a map. That way, even if the X-ray plugin is loaded, the user can still configure the SDK to use a different sampler.

In general, all of our plugin systems should work like this (propagators, span processors, etc). But looking at the spec, it does not look like we have defined a mechanism for naming and picking plugins. We just have this env var with a hard coded list of options.

I'm not sure that vendor codes need to be added to this list. Instead, we need to ensure that the SDK can still be configured via ENV vars and conf files when additional plugins have been loaded.

@anuraaga
Copy link
Contributor Author

anuraaga commented Jul 28, 2021

Yeah - I think anything marked third party as we have now, or if we want we can make the wording more explicit, are not meant to be bundled into an SDK, but that SDKs would have plugin mechanisms to retrieve from the "map of registered stuff".

There seems to be value in having names for third party plugins in the spec as well, if not in this text then maybe in a separately linked file. Again, it's all about the UX - OTel users should be able to use a consistent configuration mechanism to configure their observability, and I think this spirit applies to third party plugins. We already have the xray, ottrace propagators as an example in the spec, these samplers seem the same. Actually, @pavolloffay's change to add the configuration value in Java is on hold for the spec change because of my request - I very much don't want one language to use jaeger_remote and another jaeger, which is very easy to occur if we don't add the value to the spec. This would apply to any third party component and while our spec does take into account OSS vs proprietary in many parts, for configuration I would suggest this is not a useful distinction given the UX we want to achieve. I similarly don't want one language to call the sampler xray, another awsxray - this already happened for the propagator and that's specced! But at least with the spec it was trivial to fix the SDK without any further discussions or need for alignment.

@iNikem
Copy link
Contributor

iNikem commented Jul 28, 2021

I agree that we should have some way for different languages to agree on common values for configuration options. In the case of plugin components, that we expect vendors will develop, this may mean just saying something along these lines:

"If some configuration option allows for selecting external, 3rd party, components (such as sampler or propagator), then the specific value of that option should be provided by the component author and should be consistent across implementations of that component across different languages"?

@anuraaga
Copy link
Contributor Author

specific value of that option should be provided by the component author

@iNikem To confirm, are you suggesting adding the values to the spec, or not adding them and expecting the vendor to ensure consistency? I think because many such plugins are hosted in OTel contrib repositories (I believe it's by design), having the value defined in the spec would be helpful - in particular, it's somewhat reasonable to expect a plugin to support a vendor API be developed purely by a contributor in a language not actively checked by the vendor provider. I guess it makes sense to have the value in the spec to make sure the code in otel contrib repos is aligned (or an opentelemetry-specification-contrib? :-D)

@iNikem
Copy link
Contributor

iNikem commented Jul 28, 2021

If we expect/hope that such vendor-specific components will be developed not only by that vendor, then spec currently seems like the only place to have that value fixed.

@weyert
Copy link

weyert commented Jul 28, 2021

I can imagine to link to repo which describes these kind of samplers. I agree that vendor specific doesn't really belong in vendor agnostic specification but at the same thing we have a lot of AWS references in the semantic convention 🤔

@anuraaga
Copy link
Contributor Author

@weyert We do need to differentiate between AWS as a cloud vendor (most of the current language) and X-Ray as a tracing vendor.

Interestingly the X-Ray sampler only requires an AWS account and does not otherwise tie in to X-Ray aside form the product category. It's not charged - so I'm looking forward to the intrepid users that realize this and try using it along with ingesting traces to a totally different backend. That'd be quite cool and a great example of the interop enabled by OpenTelemetry. Given that we allow vendor code in contrib, having somewhere to have a spec to allow the contrib additions would be very great I think, it's to ensure a consistent experience for OTel users.

@SergeyKanzhelev Can you help drive this discussion?

@github-actions
Copy link

github-actions bot commented Aug 6, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 6, 2021
@anuraaga
Copy link
Contributor Author

anuraaga commented Aug 6, 2021

@SergeyKanzhelev If you don't have capacity to help with this PR do you mind reassigning? Thanks!

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read @yurishkuro's concern as we need to clearly state that these values may require additional installation. Either of them. Perhaps we can add a consistent note on this to this and propagators section.

As for the vendor values, as long as we don't include vendor specs, and refer to the external documents instead, it is all good. If we will have too much of it, we can start solving problem.

@yurishkuro please explicitly block the PR if disagree

@yurishkuro
Copy link
Member

With the _third party_ qualification it's not worse than the values defined for propagators, so I am not blocking the PR.

I agree with @SergeyKanzhelev that these non-standard values should be marked more clearly as requiring additional configuration, because the current spec can be easily interpreted by an SDK implementor that all SDKs MUST recognize these codes and include the respective 3rd party propagators/samplers in the default distro.

@SergeyKanzhelev SergeyKanzhelev merged commit c5f1a70 into open-telemetry:main Aug 6, 2021
@yurishkuro
Copy link
Member

And just a question to everyone: what is the default SDK supposed to do when it sees the env var value set to aws? Ignore it? Fail? This behavior is completely unspecified.

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants