NH-12018 Separate Distro vs Configurator

[tammy-baylis-swi]

According to this issue, every OTel Python distro must provide a Configurator and a Distro. This issue says that the Distro sets environment variables and the Configurator initializes (SDK) components.

Until this PR, our distro is doing both. It still works because auto-instrumentation loads distro entry points followed by configurator entry points -- more details in this comment. Still it would be better to separate these concerns to match what OTel Python is turning into and also how NH Java agent does it.

In this PR, SolarWindsDistro sets defaults for environments variables OTEL_PROPAGATORS, OTEL_TRACES_EXPORTER, and OTEL_TRACES_SAMPLER. This includes ensuring the CompositePropagator always starts with tracecontext,baggage,solarwinds_exporter as per this main PR comment.

EDIT 2022-04-28: In this PR, SolarWindsDistro sets defaults for environments variables OTEL_PROPAGATORS and OTEL_TRACES_EXPORTER. Please see this PR for more information on OTEL_PROPAGATORS usage: #15. At this time we can't set a default for env var OTEL_TRACES_SAMPLER as the solarwinds_sampler because it's not on OTel Python's map of _KNOWN_SAMPLERS (https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py#L364-L380) and ends up defaulting to parentbased_always_on. We instead set solarwinds_sampler as the default if customer doesn't provide OTEL_TRACES_SAMPLER env var.

EDIT 2022-05-02: The above PR changes have now been merged into this one.

The new SolarWindsConfigurator uses the env vars to initialize our sampler, trace provider, exporter, propagator, and response propagator. I thought about doing a super._configure from the parent class _OTelSDKConfigurator but didn't for three reasons:

1. We've not set up log_exporters yet
2. I'm not yet convinced that calling add_span_processor multiple times results in the same behaviour as setting up a CompositePropagator.
3. We still need to write our own code to set up the custom sampler and response propagator.

AFAIK this is the earliest point at which we can initialize liboboe Reporter / call oboe_init in the Python custom distro within auto-instrumentation. Please let me know what you think!

[tammy-baylis-swi]

Noticed a bug. Going to fix.

[tammy-baylis-swi]

Noticed a bug. Going to fix.

Done, and PR description updated.

[cheempz]

Thanks for breaking down the OTel Python startup and configuration in the PR writeup and related Jira @tammy-baylis-swi! Definitely a lot to wrap my head around but I think I get enough of it now to see your rationale with this change -- agree with the approach.

I did leave a comment, and regarding the place to initialize oboe -- is it possible to pull that out of our custom exporter and into its own method in our configurator? The reason is: sooner or later (hopefully sooner), NH will support end-to-end OTLP trace ingestion. At which point we'd no longer need a custom exporter. I know this is a bigger change, it can be in a follow-on PR if you'd like :)

[cheempz]

For the exporter and sampler above, what happens if there are multiple configured -- e..g if there were somehow two exporters configured, it seems like only the first result from next would be taken and be the one set into the BatchSpanProcesser (and similarly with sampler and TracerProvider).

[tammy-baylis-swi]

Yes I think you're right. The customer could end up somehow configuring multiples of the same name (theirs or SW exporter/sampler). Without going too far down the rabbit hole, I think the chances of this are small but non-zero if the customer is doing some funny business with their service's sys.path and/or the effective pkg_resources WorkingSet.

In 1241e7a I've switched the two iter_entry_points to load_entry_point calls, to avoid that unintended behaviour.

[cheempz]

👍

[tammy-baylis-swi]

regarding the place to initialize oboe -- is it possible to pull that out of our custom exporter and into its own method in our configurator? The reason is: sooner or later (hopefully sooner), NH will support end-to-end OTLP trace ingestion. At which point we'd no longer need a custom exporter.

Hi @cheempz , having a look! Would we only want the oboe Reporter() / _initialize_solarwinds_reporter() part of the Exporter moved to the Configurator? And keep the oboe Context and Metadata calls in the Exporter?

Or instead would it be it moving anything that uses oboe_api out of the Exporter?

Does any of this apply (either now or later) to the custom Sampler, which also uses oboe_api for Context.getDecision?

[jiwen624]

I'm curious about the propagators configuration here (I may not have fully read/understood your PR description or the Otel agent's design): I don't see a new propagator written by us here, so why can't the Otel agent do the configuration by itself?

[tammy-baylis-swi]

Ah yeah, I think I should document this all in one place instead of one PR description and one Jira comment 😅

Our new SW propagator is either named by default or must be named by OTEL_PROPAGATORS set by customer. This is implemented/policed in our SW Distro class: https://github.com/appoptics/opentelemetry-python-instrumentation-custom-distro/blob/1241e7a149facd9f8b84f274e8d3e760ffb638e5/opentelemetry_distro_solarwinds/distro.py#L30-L46

When a customer's service has NH Python installed and is launched with auto-instrumentation, this OTel Python sitecustomize.py is run. It first inits Distro class and runs distro.configure() to set up environment variables, which must include our SW propagator as above. Then it loads the configurator, which is this SW configurator. So the new propagator is included for configuration. https://github.com/open-telemetry/opentelemetry-python-contrib/blob/9e0c2ce66202e721f99a3d70389f109ec179c9a5/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py#L121-L124

Does that make sense and justify this code block? 🙂 Or more clarification required?

[jiwen624]

Sounds good to me 👍

[tammy-baylis-swi]

Thanks! 😺

I've documented this for future reference here: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2867726621/NH+Python+Troubleshooting#%E2%80%9CHow-does-NH-Python-code-get-run%3F%E2%80%9D

[tammy-baylis-swi]

Would we only want the oboe Reporter() / _initialize_solarwinds_reporter() part of the Exporter moved to the Configurator?

Right ok, we chatted about this briefly. It's something else: I'll be breaking the dependency of oboe_init on the Exporter, and doing the oboe_init elsewhere so the Sampler can still work off of an already-init'd oboe. That's why I vote for putting that in a different PR 😄 EDIT: @cheempz what do you think? Anything else for this PR?

[cheempz]

Would we only want the oboe Reporter() / _initialize_solarwinds_reporter() part of the Exporter moved to the Configurator?

Right ok, we chatted about this briefly. It's something else: I'll be breaking the dependency of oboe_init on the Exporter, and doing the oboe_init elsewhere so the Sampler can still work off of an already-init'd oboe. That's why I vote for putting that in a different PR 😄 EDIT: @cheempz what do you think? Anything else for this PR?

Sounds good!

[cheempz]

LGTM, thanks for the revisit @tammy-baylis-swi :)