Feature/urllib instrumentation by moaddib666 · Pull Request #222 · open-telemetry/opentelemetry-python-contrib

moaddib666 · 2020-11-27T08:00:33Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

tox -e test-instrumentation-urllib

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

linux-foundation-easycla · 2020-11-27T08:00:38Z

The committers are authorized under a signed CLA.

✅ Max Nikitenko (6f20f48)

toumorokoshi

Overall looks good! A couple questions around coding decisions, but nothing that would change functionality.

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

toumorokoshi · 2020-11-30T04:35:21Z

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

+    opener_open = OpenerDirector.open
+
+    @functools.wraps(opener_open)
+    def instrumented_request(opener, fullurl, data=None, timeout=None):


it might make sense to allow *args / *kwargs here, since new methods may be added that need to be passed through to open.

Does that make sense here?

Not sure, as AFAIK open does not support specific arguments open(self, fullurl, data=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT). I'd preferred left it as is. We can always add *args, **kwargs by request.

The issue with that approach is that you're fixing an issue for people once it breaks them. I'm a bit wary of inconveniencing users knowingly.

That all said, this will probably only change over major version of Python, so the likelyhood is lower.

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

toumorokoshi · 2020-11-30T04:39:37Z

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

+        if not span_name or not isinstance(span_name, str):
+            span_name = get_default_span_name(method)
+
+        recorder = URLLibInstrumentor().metric_recorder


this feels a bit weird, to assume that a class has the appropriate field instantiated without actually being a method in that class.

Any reason not to move this method into URLLibInstrumentor, or pass a reference to the metric_recorder object that would be used the by the instrumented method?

Sorry by I didn't catch the idea, metric_recorder is initialized inside main class in _instrument method. Or you are talking about a specific method like
get_metric_recorder(self): ?

I'm referring to the fact that you're instantiating a ne wURLLibInstrumentor class, just to retrieve metric_recorder. This requires a reader to know:

URLLibInstrumentor is a singleton, which requires reading more code about instrumentors

understanding that this is safe because _instrument will only be called by URLLibInstrumentor, which may not be clear to someone who modifies this code later.

It would be much clearer if you put this code in as a method into URLLibInstrumentor because:

you wouldn't have to instantiate a new URLLibInstrumentor, you could just reference it via self._metric_recorder

It's much clearer to a reader that this method is intended to only be used with URLLibInstrumentor, because it's in the class.

toumorokoshi · 2020-11-30T04:41:24Z

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

+        span_name = ""
+        if name_callback is not None:
+            span_name = name_callback()
+        if not span_name or not isinstance(span_name, str):


is this ok as a falsey check? I suppose an empty string span name being an actual desired span name is unlikely, but it might be better to just clarify the expected type here.

toumorokoshi · 2020-11-30T04:45:18Z

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

+
+                token = context.attach(
+                    context.set_value(
+                        _SUPPRESS_URLLIB_INSTRUMENTATION_KEY, True


can you clarify why this is needed? It seems to me that, as this is the request being sent, it would be impossible really to have subsequent logic also fire another span, or do something that would create such spans.

That's right, but here we only wrap this request or I've missing something ?

I believe we do this for requests because some of the request calls have the same code path (but we instrument all of them) so we add this context variable to avoid duplicates. I'm not sure if urllib has the same issue though.

We had a situation where this was needed in the botocore instrumentation to avoid infinite recursion. You can use botocore to send traces to our X-Ray backend using botocore.client('x-ray').put_trace_segments(). We had created our own exporter for the traces to not use the OTel Collector. So we would have:

Send trace using our exporter and botocore.put_trace_segments()

BotocoreInstrumentor intercepts call to botocore and generates a new trace A.

Send trace A using our exporter and botocore.put_trace_segments() to X-Ray.

BotocoreInstrumentor intercepts call to botocore and generates a new trace B.

Send trace B using our exporter and botocore.put_trace_segments() to X-Ray.

etc.

I imagine if someone exports using urllib they would encounter a similar problem.

That makes sense. But isn't the solution in that scenario to have the exporter and instrumentation honor the "suppress_instrumentation" value?

@NathanielRN in the scenario you stated, the put_trace_segements in 1 should not have emitted a trace, avoiding this whole loop. If the BotocoreInstrumentor checks for the "suppress_instrumentation" context value, it should have emitted nothing.

instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py

moaddib666 · 2020-11-30T08:39:43Z

Could we merge this code to the upstream without changes as no major issues were found.
As for now we are in the process of integration of our services with OTEL, and we must use dirty hacks to install instrumentation to our projects like:
-e git+https://github.com/moaddib666/opentelemetry-python-contrib.git@6f20f4845f9b82ade987bddc7db0df018d45f88f#egg=opentelemetry_instrumentation_urllib&subdirectory=instrumentation/opentelemetry-instrumentation-urllib ? If, so I will refactor discussed moments and add new PR during this week.

instrumentation/opentelemetry-instrumentation-urllib/CHANGELOG.md

instrumentation/opentelemetry-instrumentation-urllib/setup.cfg

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

instrumentation/opentelemetry-instrumentation-urllib/setup.cfg

...ion/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/version.py

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

lzchen · 2020-12-01T15:14:36Z

@moaddib666
I would prefer to have @toumorokoshi 's comments addressed in this PR before merging anything.

moaddib666 · 2020-12-01T17:10:13Z

@moaddib666
I would prefer to have @toumorokoshi 's comments addressed in this PR before merging anything.

Thank you, sure, no rush as we already have workaround to deploy this instrumentation.

toumorokoshi

Still have a couple changes I'd personally like to see (the URLLibInstrumentor() call in _instrumentor really bugs me), but not strong enough to request changes.

Thanks!

toumorokoshi · 2020-12-04T14:35:59Z

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

+
+                token = context.attach(
+                    context.set_value(
+                        _SUPPRESS_URLLIB_INSTRUMENTATION_KEY, True


That makes sense. But isn't the solution in that scenario to have the exporter and instrumentation honor the "suppress_instrumentation" value?

@NathanielRN in the scenario you stated, the put_trace_segements in 1 should not have emitted a trace, avoiding this whole loop. If the BotocoreInstrumentor checks for the "suppress_instrumentation" context value, it should have emitted nothing.

instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py

lzchen · 2020-12-08T18:09:43Z

@moaddib666
open-telemetry/opentelemetry-python#1438 changed metrics to instruments as part of the property in metrics. Can you rebase?

moaddib666 · 2020-12-13T12:32:38Z

@moaddib666
open-telemetry/opentelemetry-python#1438 changed metrics to instruments as part of the property in metrics. Can you rebase?

Sure, sorry for delay, I have limited internet connection right now, will update the PR early next week.

Co-authored-by: Leighton Chen <lechen@microsoft.com>

…elemetry/instrumentation/urllib/__init__.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

Co-authored-by: Leighton Chen <lechen@microsoft.com>

…elemetry/instrumentation/urllib/__init__.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

…elemetry/instrumentation/urllib/version.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

Co-authored-by: Leighton Chen <lechen@microsoft.com>

…elemetry/instrumentation/urllib/__init__.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

moaddib666 · 2020-12-14T19:40:08Z

Hi @lzchen, repo has been updated all tests were successfully passed.

lzchen · 2020-12-14T21:08:18Z

@moaddib666
Hey looks good.

Just one more thing, we've started using a consolidated top-level CHANGELOG so can you add your entry to that instead of creating a new one?

moaddib666 · 2020-12-15T18:21:41Z

@lzchen I just have email that PR was merged, thanks for your help !

moaddib666 requested review from a team, hectorhdzg and toumorokoshi and removed request for a team November 27, 2020 08:00

moaddib666 force-pushed the feature/urllib-instrumentation branch 10 times, most recently from 2cbd7ba to 6f20f48 Compare November 27, 2020 20:20

toumorokoshi approved these changes Nov 30, 2020

View reviewed changes

lzchen reviewed Nov 30, 2020

View reviewed changes

instrumentation/opentelemetry-instrumentation-urllib/CHANGELOG.md Outdated Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

instrumentation/opentelemetry-instrumentation-urllib/setup.cfg Outdated Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

instrumentation/opentelemetry-instrumentation-urllib/setup.cfg Outdated Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

instrumentation/opentelemetry-instrumentation-urllib/setup.cfg Outdated Show resolved Hide resolved

lzchen reviewed Nov 30, 2020

View reviewed changes

...ion/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/version.py Outdated Show resolved Hide resolved

moaddib666 force-pushed the feature/urllib-instrumentation branch from 46785de to ce69a70 Compare November 30, 2020 18:13

lzchen reviewed Dec 1, 2020

View reviewed changes

...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py Outdated Show resolved Hide resolved

lzchen approved these changes Dec 1, 2020

View reviewed changes

toumorokoshi approved these changes Dec 4, 2020

View reviewed changes

moaddib666 and others added 9 commits December 14, 2020 20:38

feat: add urllib instrumentation

e91af57

Update instrumentation/opentelemetry-instrumentation-urllib/setup.cfg

31375b2

Co-authored-by: Leighton Chen <lechen@microsoft.com>

Update instrumentation/opentelemetry-instrumentation-urllib/src/opent…

5b22dfd

…elemetry/instrumentation/urllib/__init__.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

Update instrumentation/opentelemetry-instrumentation-urllib/setup.cfg

cde2f29

Co-authored-by: Leighton Chen <lechen@microsoft.com>

Update instrumentation/opentelemetry-instrumentation-urllib/src/opent…

aa6f4e2

…elemetry/instrumentation/urllib/__init__.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

Update instrumentation/opentelemetry-instrumentation-urllib/src/opent…

658885a

…elemetry/instrumentation/urllib/version.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

Update instrumentation/opentelemetry-instrumentation-urllib/setup.cfg

acadb52

Co-authored-by: Leighton Chen <lechen@microsoft.com>

Update instrumentation/opentelemetry-instrumentation-urllib/src/opent…

76061b8

…elemetry/instrumentation/urllib/__init__.py Co-authored-by: Leighton Chen <lechen@microsoft.com>

fix: adjust styling issues

4c16255

moaddib666 force-pushed the feature/urllib-instrumentation branch from fb74916 to 4c16255 Compare December 14, 2020 18:42

fix: tests

5e867c5

moaddib666 and others added 3 commits December 15, 2020 10:08

docs: update changelog

5cfeea4

Merge branch 'master' into feature/urllib-instrumentation

b01cb87

Merge branch 'master' into feature/urllib-instrumentation

5c968b1

lzchen closed this Dec 15, 2020

lzchen reopened this Dec 15, 2020

Merge branch 'master' into feature/urllib-instrumentation

a054610

lzchen merged commit 187987d into open-telemetry:master Dec 15, 2020

Conversation

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

Uh oh!

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Uh oh!

linux-foundation-easycla bot commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toumorokoshi Dec 4, 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!

Uh oh!

moaddib666 commented Nov 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lzchen commented Dec 1, 2020

Uh oh!

moaddib666 commented Dec 1, 2020

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lzchen commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moaddib666 commented Dec 13, 2020

Uh oh!

moaddib666 commented Dec 14, 2020

Uh oh!

lzchen commented Dec 14, 2020

Uh oh!

moaddib666 commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

moaddib666 commented Nov 27, 2020 •

edited

Loading

linux-foundation-easycla bot commented Nov 27, 2020 •

edited

Loading

toumorokoshi Dec 4, 2020 •

edited

Loading

lzchen commented Dec 8, 2020 •

edited

Loading