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

Implement request and response hooks in requests library instrumentation #1717

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

federicobond
Copy link
Member

Description

Replaces the non-standard name_callback and span_callback with request_hook and response_hook with standard parameters. Reduced the number of instrumented functions to only one, as only Session.send is required to provide the necessary functionality.

I had to remove one test for invalid URL which should not be covered by the OTel spans. This simplifies the implementation of the instrumentor and aligns it with how the other client instrumentations work.

Fixes #411

Type of change

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

How Has This Been Tested?

  • Added integration tests for request and response hooks, modified an existing test.

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

@federicobond federicobond requested a review from a team March 10, 2023 19:51
@srikanthccv
Copy link
Member

I had to remove one test for invalid URL which should not be covered by the OTel spans.

Why should it not be covered? It's probably why we have instrumentation for Session.request. If other instrumentation do not have this either it is not possible to do the same in them, or they are missing this but capturing the invalid URL should be part of the instrumentation. This could be essential for some users.

@federicobond
Copy link
Member Author

federicobond commented Mar 12, 2023

Hi @srikanthccv I hadn't properly explained my reasoning, so thank you for bringing this up.

It's not that it should not be covered by the OTel spans, but rather that before there is a syntactically valid request object there is probably little benefit in starting a request trace. An error while building the request would still be logged to the parent span and most likely appear in an error tracking system, but there would not be a corresponding request span because one wouldn't have been attempted.

This has two added benefits: you can work with a well-built request object to append additional information during instrumentation (the use case for these hooks), and you have stricter guarantees about the values of the request span attributes that get sent to your OTel collector.

It also makes the instrumentation behavior more consistent, since invalid URL errors when manually building PreparedRequest objects are not currently logged as a span.

@srikanthccv
Copy link
Member

I think those are good points. Is there anything significant users lose if we remove Session.request wrapper?

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thanks!

@ocelotl ocelotl enabled auto-merge (squash) March 16, 2023 20:50
@ocelotl ocelotl merged commit 2629c12 into open-telemetry:main Mar 16, 2023
@federicobond federicobond deleted the requests-hooks branch March 23, 2023 04:33
@cruisehall
Copy link

@federicobond just wanted to say THANK YOU for implementing this feature -- much needed & very powerful in the right hands :)

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.

[Requests] Replace span name callback with request and response hooks
4 participants