Add support for HTTPX instrumentation by jomasti · Pull Request #461 · open-telemetry/opentelemetry-python-contrib

jomasti · 2021-04-20T02:48:07Z

Description

This builds off of the initial idea in this issue of using the
transport API in httpx. This allows for using custom transports.

This allows for manually creating transports to use with individual
clients and for instrumenting all created clients.

One issue with this current implementation is that there are few more
changes coming in 0.18.0 that this will probably want to use. So
maybe it will make sense to finalize this on that version before
releasing it. Want to get some eyes on this sooner.

Fixes#263

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added unit tests for the transports used manually and for an instrumentor class

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 · 2021-04-20T02:48:10Z

The committers are authorized under a signed CLA.

✅ Joshua (5244790)

owais

Thanks. Instrumentation LGTM. Left a few minor comments.

jomasti · 2021-04-21T04:18:17Z

The docs check fails because of a missing requirement. It's added in #463, so I will rebase and update the versions once that is merged.

owais

Thanks. Looks really good. Left a couple of questions.

codeboten · 2021-05-04T18:32:36Z

@owais have your comments been addressed?

jomasti · 2021-05-11T00:40:21Z

I have changes for 0.18.x of httpx that I still need to push up soon. Since that version formalizes the transport API, it should be the preferred base for this instrumentation.

cdvv7788 · 2021-06-01T15:00:19Z

@jomasti Anything I can help with here? Very interested on this instrumentation.

This builds off of the initial idea in [this issue][1] of using the transport API in `httpx`. This allows for using [custom transports][2]. One issue with this current implementation is that there are few more [changes coming][3] in 0.18.0 that this will probably want to use. So maybe it will make sense to finalize this on that version before releasing it. Want to get some eyes on this sooner. Resolves#263 [1]: https://github.com/encode/httpx/issues/1264#issuecomment-732034750 [2]: https://www.python-httpx.org/advanced/#custom-transports [3]: encode/httpx#1522

This is to avoid calling `get_tracer` every time.

Replaces the span and name callbacks

Simplifies the usage of request/response hooks for consumers with expected objects

Allows for instrumenting and uninstrumenting existing client instances

Not sure how that happened.

Back to only providing the raw arguments and return values instead of using httpx Request and Response objects to avoid any additional overhead or unexpected side effects.

owais

LGTM. Thanks!

owais

Looks great. Just a suggestion to use namedtuple for hook args.

Easier usage in hooks

lzchen · 2021-06-02T16:53:19Z

Have you gotten a chance to look at the instrumentation guidelines and expectations?

jomasti · 2021-06-02T18:12:40Z

Have you gotten a chance to look at the instrumentation guidelines and expectations?

I have. Is there something specific you are wanting to highlight in those?

lzchen · 2021-06-04T16:14:54Z

+ extensions=extensions,
+ )
+
+ span_attributes = _prepare_attributes(method, url)


Attributes should not be recorded if is_recording() is false.

The attributes are not recorded in this function call, but later in _apply_status_code, which checks is_recording(). Do you suggest changing this to make it more clear?

lzchen · 2021-06-04T16:18:09Z

@jomasti

It would be great if you could create issues for the features that are not included yet in this PR for this instrumentation (such as exclude_url and url_filter.

jomasti · 2021-06-04T21:12:06Z

@lzchen Isn't exclude_urls meant for the server instrumentation? The example in the file points to Flask, and the other examples in the codebase are only other server instrumentation.

lzchen · 2021-06-07T16:15:15Z

@jomasti
I don't believe that it was decided just for server instrumentations. I think it just so happens it is only implemented for server instrumentations :).

e.g. requests integration in OpenCensus has this feature.

codeboten

Just a minor comment in the setup file. @lzchen would it be ok to create a separate issue to add exclude_url functionality since the same functionality will be needed for requests as well.

codeboten · 2021-06-07T16:17:24Z

+ Programming Language :: Python :: 3
+ Programming Language :: Python :: 3.6
+ Programming Language :: Python :: 3.7
+ Programming Language :: Python :: 3.8


Should include Python 3.9

I will add it, but it looks like this is missing from almost all the other packages.

lzchen · 2021-06-07T17:33:31Z

@codeboten

Yes that's fine. @jomasti No need to add these features in this PR. It would be great if you could create issues for them to track :)

jomasti · 2021-06-08T06:10:47Z

@lzchen I can make an issue for the exclude urls feature. Looking at the functionality provided by the url filter in the example, it seems that the request hook functionality here should provide a way to accomplish the same thing. Or am I overlooking something?

- Move changelog entry - Fix license copyright - Add Python 3.9 classifier

jomasti marked this pull request as ready for review April 20, 2021 03:54

jomasti requested review from a team, owais and srikanthccv and removed request for a teamApril 20, 2021 03:54

jomasti mentioned this pull request Apr 20, 2021
httpx support #263
Closed

owais suggested changes Apr 20, 2021
View reviewed changes

owais reviewed Apr 21, 2021
View reviewed changes

Comment threadinstrumentation/opentelemetry-instrumentation-httpx/README.rst
Comment thread...tion/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py Outdated

jomasti requested a review from owais April 26, 2021 03:48

jomasti added 16 commits June 1, 2021 15:18

Update changelog
93c0f2f

Update tests
c00450e

Add instrument_client method on instrumentor
ff314c1

Store tracer on instance
650bfb7
This is to avoid calling `get_tracer` every time.

Use request and response hooks
8ff4cee
Replaces the span and name callbacks

Use httpx Request and Response classes
0eac3d0
Simplifies the usage of request/response hooks for consumers with expected objects

Use SpanAttributes from semantic conventions
7d18be7

Update documentation
603ad00

Wrap client send instead of __init__
3d83fb5
Allows for instrumenting and uninstrumenting existing client instances

Fix lint issues
3e51f1c

Fix typo in httpcore intersphinx
19fc979
Not sure how that happened.

Add clearer uninstrument client tests
9a6caed

Update versions
f27dd97

Update request and response hooks
50843a4
Back to only providing the raw arguments and return values instead of using httpx Request and Response objects to avoid any additional overhead or unexpected side effects.

Use 0.18.x as the initial base for instrumenting
5547d1f

Update based on packaging changes
9761617

owais approved these changes Jun 1, 2021
View reviewed changes

owais suggested changes Jun 1, 2021
View reviewed changes

Comment thread...tion/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py Outdated

Use namedtuple for request and response info
940097c
Easier usage in hooks

owais approved these changes Jun 1, 2021
View reviewed changes

Merge branch 'main' into feature/httpx-instrumentation
8427e31

lzchen reviewed Jun 2, 2021
View reviewed changes

Comment thread...ation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/version.py Outdated

lzchen reviewed Jun 2, 2021
View reviewed changes

Comment threadopentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py Outdated

lzchen reviewed Jun 2, 2021
View reviewed changes

Comment threadinstrumentation/opentelemetry-instrumentation-httpx/setup.cfg Outdated

Update versions
d23e247

lzchen reviewed Jun 4, 2021
View reviewed changes

Merge branch 'main' into feature/httpx-instrumentation
4ec1fc4

lzchen approved these changes Jun 4, 2021
View reviewed changes

Merge branch 'main' into feature/httpx-instrumentation
dcf9ada

codeboten approved these changes Jun 7, 2021
View reviewed changes

Updates
c8d0805
- Move changelog entry - Fix license copyright - Add Python 3.9 classifier

jomasti mentioned this pull request Jun 8, 2021
Add exclude urls feature to HTTPX instrumentation #539
Open

lzchen merged commit 9695bcf into open-telemetry:mainJun 8, 2021

jomasti deleted the feature/httpx-instrumentation branch June 8, 2021 16:08

lzchen mentioned this pull request Jan 18, 2022
Support newer httpx versions #866
Merged
7 tasks

Uh oh!

Conversation

jomasti commented Apr 20, 2021• 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-easyclaBot commented Apr 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jomasti commented Apr 21, 2021

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codeboten commented May 4, 2021

Uh oh!

jomasti commented May 11, 2021

Uh oh!

cdvv7788 commented Jun 1, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lzchen commented Jun 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jomasti commented Jun 2, 2021

Uh oh!

lzchenJun 4, 2021

Choose a reason for hiding this comment

Uh oh!

jomastiJun 4, 2021

Choose a reason for hiding this comment

Uh oh!

lzchen commented Jun 4, 2021

Uh oh!

jomasti commented Jun 4, 2021

Uh oh!

lzchen commented Jun 7, 2021

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

codebotenJun 7, 2021

Choose a reason for hiding this comment

Uh oh!

jomastiJun 8, 2021

Choose a reason for hiding this comment

Uh oh!

lzchen commented Jun 7, 2021

Uh oh!

jomasti commented Jun 8, 2021

Uh oh!

Reviewers

Assignees

Labels

jomasti commented Apr 20, 2021•
edited
Loading

linux-foundation-easyclaBot commented Apr 20, 2021•
edited
Loading

cdvv7788 commented Jun 1, 2021•
edited
Loading

lzchen commented Jun 2, 2021•
edited
Loading