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 Separate Distro vs Configurator #14

Merged

Conversation

tammy-baylis-swi
Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi commented Apr 22, 2022

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
Copy link
Contributor Author

Noticed a bug. Going to fix.

@tammy-baylis-swi
Copy link
Contributor Author

Noticed a bug. Going to fix.

Done, and PR description updated.

…g-by-customer

NH-2313 adjust CompositePropagator config with OTEL_PROPAGATORS
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.

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 :)

"Failed to load configured exporter `%s`", environ_exporter
)
raise
span_exporter = BatchSpanProcessor(exporter)
Copy link
Contributor

@cheempz cheempz May 2, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@cheempz cheempz requested a review from jiwen624 May 3, 2022 00:10
@tammy-baylis-swi
Copy link
Contributor Author

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?

span_exporter = BatchSpanProcessor(exporter)
trace.get_tracer_provider().add_span_processor(span_exporter)

# Init and set CompositePropagator globally, like OTel API
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

Sounds good to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tammy-baylis-swi
Copy link
Contributor Author

tammy-baylis-swi commented May 3, 2022

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
Copy link
Contributor

cheempz commented May 3, 2022

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!

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 for the revisit @tammy-baylis-swi :)

@tammy-baylis-swi tammy-baylis-swi merged commit 1b830a1 into add-custom-propagator May 4, 2022
@tammy-baylis-swi tammy-baylis-swi deleted the NH-12018-distro-and-configurator-setup branch May 4, 2022 00:22
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.

None yet

3 participants