Add IDs Generator as Configurable Property of Auto Instrumentation by NathanielRN · Pull Request #1404 · open-telemetry/opentelemetry-python

NathanielRN · 2020-11-20T19:22:34Z

Description

As a follow up to #1036, this PR adds a parameter to the opentelemetry-instrument executable to allow the IDs Generator to be configurable.

This is a necessary part for auto instrumentation with backends which need custom IDs, because once the opentelemetry-instrument executable sets up the TracerProvider, it is too late for the instrumented downstream application to configure the IDs.

Type of change

Please delete options that are not relevant.

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

Checklist:

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

NathanielRN · 2020-11-20T19:23:44Z

@owais Would appreciate your quick review on this so that we can have this functionality on the next release! Follow up to owais#1

opentelemetry-instrumentation/tests/test_auto_tracing.py

toumorokoshi

Thanks! This is great.

A couple small changes, but looking really good!

toumorokoshi · 2020-11-21T06:04:07Z

...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/__init__.py

    )

+    parser.add_argument(
+        "-g",


do we need a shortflag? IMO it's better to keep those to more general configuration, like version, help, etc.

Sure I agree with you! I had a hard time thinking of a letter, and so I don't think it'll be as easy to use as I had hoped, will use just the long version.

toumorokoshi · 2020-11-21T06:07:37Z

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

+    ids_generator_impl = None
+
+    for id_generator in iter_entry_points("opentelemetry_ids_generator"):
+        if id_generator.name == ids_generator_name.strip():


why deviate from the behavior of the exporters and perform a strip on the passed generator name?

Although it helps in a rare handful of cases, I think it's better to just perform no mutation on user input.

Alternatively, exporter and ids_generator should have the same mutation: strip both or strip none. It's minor, but the most confusing thing to a user is inconsistency.

I was actually under the impression that exporter did strip the value:

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

Lines 48 to 49 in 8660dea

for exp in exporter.split(","):

name = exp.strip()

Either way I think stripping the value is a good idea, I would expect

opentelemetry-instrument --ids-generator aws_xray

and

opentelemetry-instrument --ids-generator aws_xray

to work the same. But not sure if my second example would have whitespace to begin with 😅

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

toumorokoshi · 2020-11-21T06:10:32Z

opentelemetry-instrumentation/tests/test_auto_tracing.py

+    @patch.dict(
+        environ, {"OTEL_SERVICE_NAME": "my-random-ids-generator-test-service"}
+    )
+    def test_trace_init_default_random_ids_generator(self):


is it worth testing the environment variable configuring the ids_generator?

If you don't, there's a risk that someone may modify that code to not work, and we'd have no way of validating programmatically.

Yes it is worth it, I was just unfamiliar with how to do so but I agree it's worth it :) I've spent quite some time learning how to do it and finally figured out how to add it because I also think it's a useful add! Please let me know what you think!

toumorokoshi · 2020-11-21T06:12:08Z

opentelemetry-instrumentation/tests/test_auto_tracing.py

+        self.assertIsInstance(provider, Provider)
+        self.assertIsInstance(provider.ids_generator, RandomIdsGenerator)
+        self.assertIsInstance(provider.processor, Processor)
+        self.assertIsInstance(provider.processor.exporter, OTLPExporter)


why verify things like the exporter, processor, provider? It's a bit harder to see what this test is trying to verify, when you include assertions for things that aren't related.

I'd argue if you are worried about those, they should be split out into smaller tests. It helps future developers more quickly find what behavior has regressed.

I can go either way with this, I think it's useful to know that in a test where "I changed only one thing" the other parts of the functionality here still work.

However cleanliness is nice if I remove the rest of the checks, so I have removed them!

codeboten

Looks good, only question i have is non-blocking

codeboten · 2020-11-23T21:35:53Z

...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/__init__.py

    if args.service_name:
        environ["OTEL_SERVICE_NAME"] = args.service_name
+    if args.ids_generator:
+        environ["OTEL_IDS_GENERATOR"] = args.ids_generator


there should be a proposal to the spec around this env variable

Will follow up with this soon!

toumorokoshi

LGTM, thanks!

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

opentelemetry-instrumentation/tests/test_auto_tracing.py

NathanielRN requested review from a team, lzchen and toumorokoshi and removed request for a team November 20, 2020 19:22

NathanielRN force-pushed the otlp-env-config branch from 1c7fe9b to 75ff8a6 Compare November 20, 2020 19:23

NathanielRN force-pushed the otlp-env-config branch from 75ff8a6 to fcfcbf1 Compare November 20, 2020 20:12

ocelotl approved these changes Nov 20, 2020

View reviewed changes

opentelemetry-instrumentation/tests/test_auto_tracing.py Outdated Show resolved Hide resolved

NathanielRN force-pushed the otlp-env-config branch 6 times, most recently from 62bde4f to fc1224f Compare November 20, 2020 22:04

NathanielRN mentioned this pull request Nov 20, 2020

Improving Customer Experience with AWS X-Ray Auto Instrumentation Wrapper aws-observability/aws-otel-python#3

Closed

toumorokoshi suggested changes Nov 21, 2020

View reviewed changes

NathanielRN force-pushed the otlp-env-config branch 2 times, most recently from 9401751 to 70e885f Compare November 21, 2020 10:12

NathanielRN requested a review from toumorokoshi November 21, 2020 10:24

owais approved these changes Nov 21, 2020

View reviewed changes

NathanielRN mentioned this pull request Nov 21, 2020

Add AWS X-Ray Ids Generator Entry Point open-telemetry/opentelemetry-python-contrib#201

Merged

4 tasks

NathanielRN added 4 commits November 23, 2020 13:20

Add IDs Generator to auto instrumentation config

c67157d

Patch dict so delete not needed

b0f3a54

Test ids generator env var and change flag

bd71825

Generalize exports and IDs generator code

2c60b89

NathanielRN force-pushed the otlp-env-config branch from 70e885f to 2c60b89 Compare November 23, 2020 21:20

codeboten approved these changes Nov 23, 2020

View reviewed changes

Merge branch 'master' into otlp-env-config

6737421

toumorokoshi approved these changes Nov 24, 2020

View reviewed changes

...lemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/components.py Show resolved Hide resolved

opentelemetry-instrumentation/tests/test_auto_tracing.py Show resolved Hide resolved

Merge branch 'master' into otlp-env-config

c5bf367

codeboten approved these changes Nov 24, 2020

View reviewed changes

codeboten merged commit da7597c into open-telemetry:master Nov 24, 2020

NathanielRN deleted the otlp-env-config branch November 25, 2020 18:12

Conversation

NathanielRN commented Nov 20, 2020

Description

Type of change

Checklist:

Uh oh!

NathanielRN commented Nov 20, 2020

Uh oh!

Uh oh!

toumorokoshi left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanielRN Nov 21, 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!

codeboten left a comment

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!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NathanielRN Nov 21, 2020 •

edited

Loading