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

feat(urllib3)!: refactor request hook parameters #2711

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

eigenein
Copy link
Contributor

@eigenein eigenein commented Jul 16, 2024

Description

Currently, in the urllib3 instrumentation, there is no way for a request hook to inspect HTTP method and URL. This change implements forwarding the new parameters to request hooks.

Additionally, the documentation has been updated: both to represent the changes from this PR, and to fix the incorrect hook signatures.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

The unit tests have been updated to verify moving the existing body parameter and the newly added parameters.

Does This PR Require a Core Repo Change?

  • Yes.
  • 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

CHANGELOG.md Outdated Show resolved Hide resolved
@eigenein
Copy link
Contributor Author

There are failed checks in aiopg integration that I haven't touched. Something's wrong with its test-requirements.txt:

ERROR: Could not find a version that satisfies the requirement install==1.3.5 (from versions: none)
ERROR: No matching distribution found for install==1.3.5

Perhaps, it should be pip-install==1.3.5?

@eigenein eigenein marked this pull request as ready for review July 16, 2024 14:56
@eigenein eigenein requested a review from a team July 16, 2024 14:56
@emdneto
Copy link
Member

emdneto commented Jul 16, 2024

There are failed checks in aiopg integration that I haven't touched. Something's wrong with its test-requirements.txt:

ERROR: Could not find a version that satisfies the requirement install==1.3.5 (from versions: none)
ERROR: No matching distribution found for install==1.3.5

Perhaps, it should be pip-install==1.3.5?

Fix for this is here See #2712

@eigenein eigenein force-pushed the main branch 2 times, most recently from 2aa6be1 to cab3f75 Compare July 16, 2024 17:57
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@eigenein eigenein force-pushed the main branch 4 times, most recently from 7fe5ac2 to 2a8fde2 Compare July 19, 2024 13:04
@eigenein eigenein changed the title feat(urllib3)!: add method and url parameters to the request hook feat(urllib3)!: refactor request hook to also include method and url parameters Jul 19, 2024
@eigenein eigenein changed the title feat(urllib3)!: refactor request hook to also include method and url parameters feat(urllib3)!: refactor request hook parameters Jul 22, 2024
@eigenein eigenein force-pushed the main branch 5 times, most recently from ff5a3df to 3ef65fb Compare July 22, 2024 17:29
@eigenein
Copy link
Contributor Author

@lzchen Done and rebased, ready for review

I decided to keep the pool parameter because of this note:

headers (Mapping[str, str] | None) – Dictionary of custom headers to send, such as User-Agent, If-None-Match, etc. If None, pool headers are used. If provided, these headers completely replace any pool-specific headers.

So, the hook may still want to access pool.headers

CHANGELOG.md Outdated Show resolved Hide resolved
@eigenein
Copy link
Contributor Author

Alright, I'm keeping the docs "as is". Adjusted the changelog and rebased. Ready for merge

@lzchen lzchen merged commit 4f98519 into open-telemetry:main Jul 25, 2024
383 checks passed
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.

5 participants