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

Tracer.capture_method decorator does not preserve function type hints #465

Closed
zlalvani opened this issue Jun 7, 2021 · 5 comments
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@zlalvani
Copy link

zlalvani commented Jun 7, 2021

What were you trying to accomplish?
I want to use the @tracer.capture_method decorator for detailed function level tracing however it's currently unusable because it disables all errors when linting with mypy.

Expected Behavior

See code example below

Current Behavior

Function signatures (argument and return types) are not preserved when decorated with @tracer.capture_method and linting with mypy.

Possible Solution

Mypy can be forced to look at the decorated function signature: https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators

Steps to Reproduce (for bugs)

  1. Use code sample below
  2. Run mypy (tested on mypy==0.782)

Environment

  • Powertools version used: aws-lambda-powertools==1.10.2
  • Packaging format (Layers, PyPi): PyPi

How to enable debug mode**

tracer = Tracer()

@tracer.capture_method
def foo(bar: str) -> str:
    return bar

x = foo('baz')
x.get('this is not a dict')  # should cause mypy error
y = foo(123)  # should cause mypy error
@zlalvani zlalvani added bug Something isn't working triage Pending triage from maintainers labels Jun 7, 2021
@heitorlessa heitorlessa added the help wanted Could use a second pair of eyes/hands label Jun 8, 2021
@heitorlessa
Copy link
Contributor

Hey @zlalvani thanks a lot for raising this. While we've had numerous MyPy issues fixed we don't yet have full support for it -- Roadmap item that you can help us +1 to prioritize: aws-powertools/powertools-lambda#3

I'm unsure how to fix this atm as I had thought functools.wraps would ensure some level of correctness here (not at wrapt level, tho) -- This is that part of the code: https://github.com/awslabs/aws-lambda-powertools-python/blob/8724294013e3157de9478f90e7c1283a952de54f/aws_lambda_powertools/tracing/tracer.py#L607

I've been meaning to refactor capture_method as it's the most complex logic we have in the codebase, so it'd be a good exercise nevertheless to get this improved.

I'd appreciate if you have any guidance on how we can satisfy MyPy in a backwards compatible way - happy to provide any guidance if you'd like to try contributing it.

Thank you!

@heitorlessa heitorlessa added area/tracer documentation Improvements or additions to documentation triage Pending triage from maintainers and removed bug Something isn't working help wanted Could use a second pair of eyes/hands triage Pending triage from maintainers labels Jul 2, 2021
@heitorlessa heitorlessa self-assigned this Jul 2, 2021
@heitorlessa heitorlessa added area/tracer bug Something isn't working help wanted Could use a second pair of eyes/hands and removed documentation Improvements or additions to documentation area/tracer labels Jul 2, 2021
@heitorlessa
Copy link
Contributor

heitorlessa commented Jul 16, 2021

Updating here to let you know the fix will be available in the next release.

Update: Spoke too soon. While this works it breaks when using decorator parameters - error: <nothing> not callable. The closest workaround is this but not suitable to what we're doing -- needs further investigation:

python/mypy#1551 (comment)

offending block: https://github.com/awslabs/aws-lambda-powertools-python/blob/5b87bb195fb154d2a112364a5d1d5c9513898e55/aws_lambda_powertools/tracing/tracer.py#L490

Potential solution is to use an overload like Pydantic does: pydantic/pydantic#2052

@heitorlessa heitorlessa removed help wanted Could use a second pair of eyes/hands triage Pending triage from maintainers labels Jul 16, 2021
@heitorlessa heitorlessa added this to the 1.18.0 milestone Jul 16, 2021
@heitorlessa
Copy link
Contributor

Just merged the fix for synchronous functions - Async however requires the new ParamSpec only available in 3.10, so I'll wait for more customers using mypy + async fn as it'll increase the package size by 240K.

This will be available in next week's release (1.18.0) -- I'll ping back here once it's released.

Thank you again for flagging it @zlalvani

@heitorlessa heitorlessa added pending-release Fix or implementation already in dev waiting to be released and removed pending-release Fix or implementation already in dev waiting to be released labels Jul 17, 2021
@heitorlessa
Copy link
Contributor

Hi @zlalvani - This is now fixed in 1.18.0

More details in the release notes: https://github.com/awslabs/aws-lambda-powertools-python/releases/tag/v1.18.0

@zlalvani
Copy link
Author

Awesome thanks for the fix! @heitorlessa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants