Automatic exporter/provider setup for opentelemetry-instrument command. by owais · Pull Request #1036 · open-telemetry/opentelemetry-python

owais · 2020-08-23T23:16:31Z

This PR adds support to the opentelemetry-instrument command to automatically configure a tracer provider and exporter. By default configures the OTLP exporter (like other Otel auto-instrumentations. e.g, Java: https://github.com/open-telemetry/opentelemetry-java-instrumentation#getting-started). It also allows using a different in-built or 3rd party via a CLI argument or env variable.

Details can be found on opentelemetry-instrumentation's README package.

Fixes #663

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Ships with automated tests
Tested with a demo app

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

open-telemetry#1036 This commit adds support to the opentelemetry-instrument command to automatically configure a tracer provider and exporter. By default, it configures the OTLP exporter (like other Otel auto-instrumentations. e.g, Java: https://github.com/open-telemetry/opentelemetry-java-instrumentation#getting-started). It also allows using a different in-built or 3rd party via a CLI argument or env variable. Details can be found on opentelemetry-instrumentation's README package. Fixes open-telemetry#663

owais · 2020-10-02T15:06:49Z

Update:

This PR now only support two configuration options.

--exporter or OTEL_EXPORTER

This options takes a comma separated list of exporter names and automatically configures these exporters. In addition to the well-known exporters (ones present in this repo), it also accepts qualified Python paths to valid exporters. It accepts metric exporters but does nothing with them right now. Support for auto configuring metrics pipeline will be added later.

--service-name or SERVICE_NAME

This value is used to configure the given exporter and/or provider with a service name.

@open-telemetry/python-approvers

owais · 2020-10-05T23:04:58Z

Further simplified this by removing new features added to the bootstrap command (ability to install exporters). I'll follow up with a separate PR for that.

opentelemetry-instrumentation/README.rst

ocelotl · 2020-10-14T20:22:39Z

...lemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/components.py

+}
+
+
+def _import(import_path: str) -> any:


May I recommend not relying on import paths but to use standard entry points? This is an example.

ocelotl · 2020-10-14T20:31:20Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+
+if (
+    hasattr(sys, "argv")
+    and sys.argv[0].split(os.path.sep)[-1] == "celery"


Can this be moved to the celery instrumentation package?

We could do that and then pass argv to a function from the celery instrumentation package but I don't see much benefit in that. It is a special case in real life so probably we should live with the fact and allow it. Also, we initialize entire tracing pipeline here and instrument all packages, not just celery so not sure if celery instrumentation having this would be the right choice.

Also, this is orthogonal to celery instrumentation really. Users might not have celery instrumentation installed but might still want their celery process to be traced (manually or through other instrumentation packages) and that's what this helps with. Without this, batch span exporter does not work with celery workers.

This PR has been open for a while, so in order to move forward I am ok with this as it is right now. Nevertheless, I think we should definitely keep the "core" instrumentation completely separated from the implementation details of any and all of the instrumentation packages. This probably means allowing all the instrumentation packages to be allowed to run a preregistered function or something similar that includes this kind of code.

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

...lemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/components.py

ocelotl · 2020-10-14T20:45:20Z

...lemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/components.py

+
+_DEFAULT_EXPORTER = symbols.exporter_otlp
+
+known_exporters = {


If I understand this correctly, this is mapping certain strings to an import path of classes that already exist in our repo, with the purpose of instantiating these classes when their matching keys are passed as arguments via CLI. Using entry points would save us from having to implement this by hand, as this is basically what the entry points mechanism does, it associates a certain string defined in the setup.cfg to a Python object, which can be the class. This approach has the advantage that it keeps the plugins (the code related to the specific library or exporter in this case) separated from the code of the "core" opentelemetry-instrumentation component.

Thanks. That might work. I like that it means installing any compatible package will automatically add support for the specific package to the instrument command. I'll see if it has everything we need.

NathanielRN · 2020-10-22T04:44:37Z

Hello! I have a PR to move some files you have in this PR to the Contrib repo, please let me know if this gets merged before the PR in the Contrib repo. Please see https://github.com/open-telemetry/opentelemetry-python-contrib/pulls/83

owais · 2020-10-22T15:54:04Z

@NathanielRN This PR is only touching the base instrumentation package. I believe this is staying in core and not moving to contrib or is the plan to move this as well?

NathanielRN · 2020-10-22T18:21:36Z

@owais I see that it's also changing instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py which would be moving to the Contrib Repo in open-telemetry/opentelemetry-python-contrib#83 so just want to make sure we catch that change so that it makes it into the Contrib master.

owais · 2020-10-22T18:53:17Z

It's just auto-reformat by black. I'll drop these changes when I refactor so nothing to worry about. @NathanielRN

owais · 2020-11-10T00:10:48Z

@ocelotl updated to use entry points. Thanks for the tip.

owais · 2020-11-10T00:15:50Z

In order to simplify this and use iter points to not hard code knowledge about exporters, I had to temporarily remove support for Datadog exporter. My understanding is that GA does not require this feature to support contrib or 3rd party exporters so we should be good to go. I'll follow up with a separate PR to allow special cases like the Datadog exporter to work with this feature as well.

ocelotl

Requesting just a typo to be fixed 👍

opentelemetry-instrumentation/README.rst

ocelotl · 2020-11-11T22:22:06Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+
+if (
+    hasattr(sys, "argv")
+    and sys.argv[0].split(os.path.sep)[-1] == "celery"


This PR has been open for a while, so in order to move forward I am ok with this as it is right now. Nevertheless, I think we should definitely keep the "core" instrumentation completely separated from the implementation details of any and all of the instrumentation packages. This probably means allowing all the instrumentation packages to be allowed to run a preregistered function or something similar that includes this kind of code.

ocelotl

LGTM, thanks @owais!

NathanielRN · 2020-11-18T17:22:58Z

Really excited for this change! I added a PR to this branch to add the ability to set a Custom IDs Generator, would be nice to get these configs all in at once! owais#1

NathanielRN · 2020-11-18T17:31:58Z

opentelemetry-instrumentation/CHANGELOG.md

+- Fixed boostrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))
+- Added support for `OTEL_EXPORTER` to the `opentelemetry-instrument` command ([#1036](https://github.com/open-telemetry/opentelemetry-python/pull/1036))


We should probably move the PR references to be on a new line to be consistent with the other Changelog messages.

NathanielRN · 2020-11-18T17:34:36Z

opentelemetry-instrumentation/README.rst

+of the well-known exporter names (see below) or a fully
+qualified Python import path to a span  exporter implementation.


Does this mean I can set exporter opentelemetry.exporter.jaeger:JaegerSpanExporter?

Sorry for my misunderstanding, but when I look at the code it looks like it will error out if it doesn't match ep.name (where ep is an entry point).

exporters = { ep.name: ep for ep in iter_entry_points("opentelemetry_exporter") } for exporter_name in exporter_names: entry_point = exporters.get(exporter_name, None) if not entry_point: raise RuntimeError( "Requested exporter not found: {0}".format(exporter_name) )

So doesn't it have to be otlp?

You're right. This PR supported what the documentation says but I ended up removing it in order to simplify the implementation. Looks like I missed this piece of documentation when I removed the feature.

NathanielRN · 2020-11-18T17:36:58Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+            logger.debug("Instrumented %s", entry_point.name)
+
+        except Exception:  # pylint: disable=broad-except
+            logger.exception("Instrumenting of %s failed", entry_point.name)


Should this re-raise the Exception?

I'm not sure if this case should halt execution of the program and IIRC, any exceptions raised inside sitecustomize are "eaten up" by python by default and the process still continues to work.

I meant because you only call auto_instrument in line 41 below which is wrapped in another try except block.

So if you re raise it we will get the "Failed to auto initialize opentelemetry" log which will be true in a sense.

Ah, makes sense.

NathanielRN · 2020-11-18T17:38:20Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+if (
+    hasattr(sys, "argv")
+    and sys.argv[0].split(os.path.sep)[-1] == "celery"
+    and "worker" in sys.argv


Can this be the following? Not sure if it's more performant or less...

Suggested change

and "worker" in sys.argv

and "worker" in sys.argv[1:]

Don't think it matter either way. [1:] probably clarifies the intent a tiny bit more so will make the change.

NathanielRN · 2020-11-18T17:48:29Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+    from celery.signals import worker_process_init  # pylint:disable=E0401
+
+    @worker_process_init.connect(weak=False)
+    def init_celery(*args, **kwargs):
+        initialize()


This is a sweet catch, thanks for knowing to do something like this!

NathanielRN

Really excited for this change 👍

codeboten

This looks pretty good, just a minor typo to fix and a question about the change in docs.

codeboten · 2020-11-19T19:55:31Z

opentelemetry-instrumentation/CHANGELOG.md

 Released 2020-10-13

- Fixed boostrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask
+- Fixed boostrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))


Suggested change

- Fixed boostrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))

- Fixed bootstrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))

codeboten · 2020-11-19T19:56:16Z

opentelemetry-instrumentation/README.rst

 ::

-    opentelemetry-bootstrap --action=install|requirements
+    opentelemetry-instrument -e zipkin,otlp celery -A tasks worker --loglevel=info


can you explain the reason behind removing the info about opentelemetry-bootstrap?

+1, this info is probably important to keep here.

This is unintentional. Probably caused by a bad rebase. Fixing.

This commit extends the instrument command so it automatically configures tracing with a provider, span processor and exporter. Most of the component used can be customized with env vars or CLI arguments. Details can be found on opentelemetry-instrumentation's README package. Fixes open-telemetry#663

codeboten

Thanks for fixing

owais requested a review from a team August 23, 2020 23:16

owais force-pushed the otlp-env-config branch 2 times, most recently from 1568630 to 05a1fed Compare August 29, 2020 15:05

owais changed the title ~~POC: actual auto-instrumentation~~ Automatic exporter/provider setup for opentelemetry-instrument command. Aug 29, 2020

owais force-pushed the otlp-env-config branch 2 times, most recently from 53a5233 to 0ec2526 Compare August 29, 2020 15:10

owais force-pushed the otlp-env-config branch from 0ec2526 to 29000e8 Compare August 29, 2020 15:21

owais force-pushed the otlp-env-config branch from 29000e8 to 9b72eda Compare August 29, 2020 15:27

owais force-pushed the otlp-env-config branch from 9b72eda to 0e93d67 Compare August 29, 2020 15:29

owais force-pushed the otlp-env-config branch from 0e93d67 to 1fcd121 Compare August 29, 2020 15:41

owais force-pushed the otlp-env-config branch from 1fcd121 to e2e6ddf Compare August 29, 2020 16:16

owais force-pushed the otlp-env-config branch from e2e6ddf to 1a35dc3 Compare August 29, 2020 16:16

owais force-pushed the otlp-env-config branch from 1a35dc3 to 72369ff Compare August 29, 2020 16:20

owais force-pushed the otlp-env-config branch from 72369ff to 536e913 Compare August 29, 2020 22:40

owais force-pushed the otlp-env-config branch from 536e913 to e75a7e8 Compare August 29, 2020 23:01

owais force-pushed the otlp-env-config branch from e75a7e8 to a79536e Compare August 29, 2020 23:27

owais force-pushed the otlp-env-config branch from a79536e to a8ef8aa Compare August 29, 2020 23:32

owais force-pushed the otlp-env-config branch from a8ef8aa to 4c17c20 Compare August 29, 2020 23:41

owais force-pushed the otlp-env-config branch 5 times, most recently from dac8060 to e2a1a04 Compare October 2, 2020 14:45

ocelotl suggested changes Oct 14, 2020

View reviewed changes

ocelotl suggested changes Nov 11, 2020

View reviewed changes

ocelotl approved these changes Nov 13, 2020

View reviewed changes

NathanielRN reviewed Nov 18, 2020

View reviewed changes

NathanielRN approved these changes Nov 18, 2020

View reviewed changes

codeboten reviewed Nov 19, 2020

View reviewed changes

codeboten approved these changes Nov 20, 2020

View reviewed changes

This was referenced Nov 20, 2020

Add IDs Generator as Configurable Property of Auto Instrumentation #1404

Merged

Instrumentation Package depends on the OTel SDK #1405

Merged

codeboten mentioned this pull request Nov 26, 2020

Remove SDK dependency from instrumentation package #1421

Closed

toumorokoshi mentioned this pull request Nov 30, 2020

Documentation of default exporters open-telemetry/opentelemetry.io#4348

Open

owais mentioned this pull request Dec 3, 2020

Extend exporter interface to include initializer (__init__) or introduce a new exporter factory interface #1442

Closed

NathanielRN mentioned this pull request May 18, 2022

Auto Instrumentation Needs Support for Initializing Metrics open-telemetry/opentelemetry-python-contrib#1096

Closed

adamantike mentioned this pull request Mar 27, 2021

How to use auto-instrumentation for Uvicorn with multiple worker processes open-telemetry/opentelemetry-python-contrib#385

Closed


		_DEFAULT_EXPORTER = symbols.exporter_otlp

		known_exporters = {

		- Fixed boostrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))
		- Added support for `OTEL_EXPORTER` to the `opentelemetry-instrument` command ([#1036](https://github.com/open-telemetry/opentelemetry-python/pull/1036))

		of the well-known exporter names (see below) or a fully
		qualified Python import path to a span exporter implementation.

	- Fixed boostrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))
	- Fixed bootstrap command to correctly install opentelemetry-instrumentation-falcon instead of opentelemetry-instrumentation-flask. ([#1138](https://github.com/open-telemetry/opentelemetry-python/pull/1138))

Conversation

owais commented Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

owais commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update:

Uh oh!

owais commented Oct 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanielRN commented Oct 22, 2020

Uh oh!

owais commented Oct 22, 2020

Uh oh!

NathanielRN commented Oct 22, 2020

Uh oh!

owais commented Oct 22, 2020

Uh oh!

owais commented Nov 10, 2020

Uh oh!

owais commented Nov 10, 2020

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

NathanielRN commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owais Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanielRN left a comment

Choose a reason for hiding this comment

Uh oh!

owais commented Aug 23, 2020 •

edited

Loading

owais commented Oct 2, 2020 •

edited

Loading

owais Nov 18, 2020 •

edited

Loading