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(asgi,fastapi,starlette)!: provide both send and receive hooks with scope and message #2546

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

eigenein
Copy link
Contributor

@eigenein eigenein commented May 23, 2024

Description

Currently, a «receive» hook is only provided with scope, but not message. On contrary, a «send» hook is only provided with message, but not scope.

This change unifies the both hooks and provides them with additional request info.

This is a breaking change:

  • The hooks will now be provided with one more positional argument
  • client_request_hook will be called after receive() instead of before receive()

Resolves #2560.

Type of change

  • 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?

Existing unit tests have been updated.

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

Copy link

linux-foundation-easycla bot commented May 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: eigenein / name: Pavel Perestoronin (8010527)

@eigenein eigenein marked this pull request as ready for review May 23, 2024 15:24
@eigenein eigenein requested a review from a team as a code owner May 23, 2024 15:24
@eigenein eigenein force-pushed the main branch 3 times, most recently from 3336bca to eaacb51 Compare May 27, 2024 12:34
@eigenein
Copy link
Contributor Author

Fixed the linter errors, let's try it again

@eigenein
Copy link
Contributor Author

Ready for review

@tammy-baylis-swi
Copy link
Contributor

Thank you for submitting this! A couple of asks:

client_request_hook will be called after receive() instead of before receive()

(1) How has this been tested? Does the hook still work the same way to e.g. set span name or attributes?

(2) Please could you also create and link a github Issue describing the feature, like how this PR links this issue, especially because this would be a breaking change.

@eigenein
Copy link
Contributor Author

eigenein commented May 27, 2024

Sure, thanks for looking into it!

  1. There are existing tests for hooks, that verify the hooks keep working the same way (they test exactly setting span name & attributes, and would've gone red should I break anything):

    Of course, they went red because I added the new parameters, so I just updated the test hooks in there.

    The call order change here basically ensures that the hook could access the ASGI event. No functional changes, otherwise. I don't think the call order was part of the public interface, but pointed it out – just in case.

  2. Would you like me to just copy-paste the description, or would you need some additional info in there? This pull request is pretty self-contained, there was no bug or feature request in the first place.

@tammy-baylis-swi
Copy link
Contributor

The call order change here basically ensures that the hook could access the ASGI event. No functional changes, otherwise. I don't think the call order was part of the public interface, but pointed it out – just in case.

Great thanks!

  1. Would you like me to just copy-paste the description, or would you need some additional info in there? This pull request is pretty self-contained, there was no bug or feature request in the first place.

Yes please a copy-paste works 🙂

Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi left a comment

Choose a reason for hiding this comment

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

Also Lgtm, nice to see those types in shared ASGI module. We'll get eyes from the maintainers as well.

@eigenein eigenein changed the title feat(asgi,fastapi,starlette)!: provide both send and receive hooks with scope and message feat(asgi,fastapi,starlette)!: provide both send and receive hooks with scope and message, resolves #2560 May 28, 2024
@eigenein eigenein changed the title feat(asgi,fastapi,starlette)!: provide both send and receive hooks with scope and message, resolves #2560 feat(asgi,fastapi,starlette)!: provide both send and receive hooks with scope and message May 28, 2024
@eigenein
Copy link
Contributor Author

Done!

CHANGELOG.md Outdated Show resolved Hide resolved
…th `scope` and `message`

Currently, a «receive» hook is only provided with `scope`, but not `message`. On contrary, a «send» hook is only provided with `message`, but not `scope`.

This change unifies the both hooks and provides them with additional request info.

This is a breaking change:

- The hooks will now be provided with one more positional argument
- `client_request_hook` will be called _after_ `receive()` instead of _before_ `receive()`
@eigenein
Copy link
Contributor Author

eigenein commented Jun 3, 2024

Rebased and corrected the changelog

@lzchen lzchen merged commit ed51ebb into open-telemetry:main Jun 3, 2024
314 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.

ASGI, FastAPI, Starlette: provide both send and receive hooks with scope and message
3 participants