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

Add hooks for aiohttp, asgi, starlette, fastAPI, urllib, urllib3 #576

Merged
merged 19 commits into from
Jul 26, 2021

Conversation

ryokather
Copy link
Contributor

@ryokather ryokather commented Jul 12, 2021

Description

This PR implements request and response hooks for the remaining web framework instrumentations excluding the requests instrumentation (see notes). These hooks enable users to customize behavior (ex: update span name), record additional information, and/or allow injecting tracing-related data from incoming/outgoing requests/responses. Hooks were implemented in line with what was previously implemented in other instrumentations as well as what was specified in issue #408.

Any previous usage of a span_name or span_detail callback was removed as its functionality of customizing a span_name is achievable through request/response_hooks.

Additionally, in the asgi instrumentation the existing span_details_callback was kept and renamed to default_span_details. This is because the FastAPI and Starlette instrumentation, which utilizes the asgi framework, requires all created spans to use FastAPI/Starlette routes as the span name. This function essentially provides the ability to configure the default span name that all spans would use.

This PR fixes issues #135, #136, #412 in addition to implementing hooks for the asgi and aiohttp-client instrumentations.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Any previous usage of span_name modifications would not work. Instead, implemented request and response hooks should be utilized.

How Has This Been Tested?

An additional test called test_hooks was implemented in each of the updated instrumentations: aiohttp-client, asgi, fastapi, starlette, urllib, urllib3. Tests were executed locally using tox.

Notes

  1. Hooks were not implemented for the request instrumentation [Requests] Replace span name callback with request and response hooks #411. This is because the wrapper for Session.request provides no ability to access a request-like object which is necessary for a request_hook so this instrumentation was left for further discussion.
  2. In the Starlette and FastAPI instrumentations, the asynchronous function wrapped_recieve() is never invoked so a receive_span is never created. This means that the client_request_hook is never called and thus this was not tested in the Starlette and FastAPI instrumentations. However, this functionality is tested in the asgi framework and is deemed to work.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • 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

@ryokather ryokather requested review from a team, codeboten and srikanthccv and removed request for a team July 12, 2021 17:34
from opentelemetry.util.http import get_excluded_urls

_excluded_urls = get_excluded_urls("STARLETTE")

_ServerRequestHookT = typing.Optional[typing.Callable[[Span, dict], None]]
_ClientRequestHookT = typing.Optional[typing.Callable[[Span, dict], None]]
_ClientResponseHookT = typing.Optional[typing.Callable[[Span, dict], None]]
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if it makes sense to move these to the base instrumentation code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo they shouldn't because each instrumentation can have different types for the hooks e.g. Urllib has the following types. Starlette and FastAPI have the same types as they both use aiohttp

_RequestHookT = typing.Optional[
     typing.Callable[[Span, urllib3.connectionpool.HTTPConnectionPool], None]
 ]
 _ResponseHookT = typing.Optional[
     typing.Callable[
         [
             Span,
             urllib3.connectionpool.HTTPConnectionPool,
             urllib3.response.HTTPResponse,
         ],
         None,
     ]
 ] 

CHANGELOG.md Outdated
@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.4.0-0.23b0...HEAD)

### Changed
- [576] `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`, `opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks ([#576](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/576))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [576] `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`, `opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks ([#576](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/576))
- `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`, `opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks ([#576](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/576))

CHANGELOG.md Outdated
@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.4.0-0.23b0...HEAD)

### Changed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Changed
### Added

@ryokather
Copy link
Contributor Author

@lzchen updated the CHANGELOG format. Let me know if there's anything we can do about the docs issue

@lzchen
Copy link
Contributor

lzchen commented Jul 26, 2021

@ryokather
Please fix the build and then we can get this merged.

@ryokather
Copy link
Contributor Author

@lzchen docs issue fixed 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants