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 instrumentor and auto instrumentation support for aiohttp #1075

Merged
merged 4 commits into from
Oct 9, 2020

Conversation

mariojonke
Copy link
Contributor

@mariojonke mariojonke commented Sep 4, 2020

Description

Current aiohttp-client instrumentation requires to explicitly create a TraceConfig and pass it to the constructor of a ClientSession.
This PR introduces an instrumentor for aiohttp ClientSessions which automatically adds a TraceConfig when a ClientSession is created.
Also adds the entry point for auto-instrumenting aiohttp client sessions.

Type of change

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

How Has This Been Tested?

Manual tests + additional unit tests

Checklist:

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

* add an instrumentor for aiohttp client sessions
* the instrumentor will wrap the ClientSession constructor
  and automatically a TraceConfig on construction
@mariojonke mariojonke requested a review from a team September 4, 2020 06:42
@codeboten codeboten added the instrumentation Related to the instrumentation of third party libraries or frameworks label Sep 8, 2020
@lzchen lzchen self-assigned this Oct 1, 2020
"""
# pylint:disable=unused-argument
def instrumented_init(wrapped, instance, args, kwargs):
if context_api.get_value("suppress_instrumentation"):
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have this check in instrumented methods. Does this exist to prevent instrumenting libraries in the first place or to prevent generating spans for certain code paths? I think it's the later. In other words, I think we should still instrument the libraries but instrumentation itself should check for this for each request and skip generating spans in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once AioHttpClientInstrumentor().instrument() is called, the library is always instrumented. this check just supresses instrumentation for a single ClientSession.
aiohttp already comes with built in hooks for instrumenting certain aspects. To make use of these hooks one has to pass one or more TraceConfig objects to the ClientSession constructor. The AioHttpClientInstrumentor piggybacks on this mechanism by wrapping the ClientSession constructor to automatically add a TraceConfig with the respective hooks every time a ClientSession is created. The suppress_instrumentation is to exclude single ClientSessions from being instrumented.

@codeboten codeboten assigned ocelotl and unassigned lzchen Oct 8, 2020
span_name=span_name,
tracer_provider=tracer_provider,
)
trace_config.opentelemetry_aiohttp_trace_config = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps something like trace_config.opentelemetry_aiohttp_instrumented is more descriptive?

@lzchen lzchen merged commit c7cc4d9 into open-telemetry:master Oct 9, 2020
@mariojonke mariojonke deleted the aiohttp-instrumentor branch November 26, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instrumentation Related to the instrumentation of third party libraries or frameworks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants