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

NH-12018 oboe Reporter init moved to Configurator #17

Merged

Conversation

tammy-baylis-swi
Copy link
Contributor

This update moves oboe Reporter() from the SW Exporter to the SW Configurator so that oboe_init is called at custom-distro setup when using the SW Exporter or with a different exporter set by OTEL_TRACES_EXPORTER env var. There doesn't seem to be any other way to trigger oboe_init in the SWIG'd oboe_api, which makes sense as it looks like the only responsibilities of Reporter() are to assemble options then use them to oboe_init.

This also reverts a change made in #14 so that instead of load_entry_point for a non-SW exporter, we go back to:

exporter = next(
    iter_entry_points(
        "opentelemetry_traces_exporter",
        environ_exporter_name
    )
).load()()

I've done this because in my mind it's the cleanest way for custom-distro to find something like otlp_proto_grpc in the Python Distribution at runtime. It's too brittle to map required dist names for load_entry_point, and it's too complex and a waste to implement a search through sys.path when iter_entry_points already does it. Even though there is a small chance a customer might have multiples of the same environ_exporter_name in their path (see this comment), generally speaking that's for them to make sure is correct. Also, the iter_entry_points approach is used all over OTel Python and it's cleaner to match than to mix up setup techniques.

Special note: Currently OTel Python does not use entry points to configure Sampler like it does for Exporter and Propagators, so using iter_entry_points or load_entry_point for a non-SW Sampler fails. OTel Python uses sampling._get_from_env_or_default() so we do too: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py#L374-L377 I've added this piece to the NH Python Troubleshooting doc.

Here is today's manual testing doc for this update: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3042050066/2022-05-04+configurator+testing

Please let me know what you think or if I've missed something 😃

@tammy-baylis-swi tammy-baylis-swi requested a review from cheempz May 4, 2022 22:18
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

As always thank you for the informative PR writeup @tammy-baylis-swi! The changes LGTM with one caveat which is my fault for not having actually discussed it with you -- let's essentially pin our distro to ONLY use our custom sampler :). Added the rationale to your nice confluence page.

And indeed I see now there are several cases of the use of next(iter_entry_points().. outside of any type of looping in OTel Python, and it does make sense from the angle of working around the brittleness of specifying a dist :)

@tammy-baylis-swi
Copy link
Contributor Author

let's essentially pin our distro to ONLY use our custom sampler :)

Thank you @cheempz ! Changed to always use our SW Sampler in 01f1e98. Setting OTEL_TRACES_SAMPLER now gets ignored.

I've also changed the Troubleshooting page back to "Not supported" for customizing OTel sampler. I kept my findings in a note in case we want to try something similar in the future.

Please let me know what you think 😺

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tammy-baylis-swi! I've been glossing over how the agents should handle fatal misconfiguration but this PR has reminded me to jot it down in https://swicloud.atlassian.net/browse/NH-11751 so we can tackle that separately.

@tammy-baylis-swi tammy-baylis-swi merged commit 7e758df into add-custom-propagator May 5, 2022
@tammy-baylis-swi tammy-baylis-swi deleted the NH-12018-oboe-reporter-in-configurator branch May 5, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants