-
Notifications
You must be signed in to change notification settings - Fork 79
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 logfire.instrument_aws_lambda
#657
Conversation
Deploying logfire-docs with Cloudflare Pages
|
5cab65b
to
202a692
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #657 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 135 137 +2
Lines 10685 10735 +50
Branches 1466 1471 +5
=========================================
+ Hits 10685 10735 +50 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What have you learned from manual testing? Didn't it not work in the python runtime?
I think basic automated testing like asserting that something got patched is good enough.
docs/integrations/aws-lambda.md
Outdated
``` | ||
|
||
1. Remember to set the `LOGFIRE_TOKEN` environment variable on your Lambda function configuration. | ||
2. The `logfire.instrument_aws_lambda` function must be called after defining the handler function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be enforced with code, e.g. require calling logfire.instrument_aws_lambda(handler)
or check globals()['handler']
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do it with globals()
, but the function was not there, I guess I needed to go up the stack and get the locals up? It was easier to add the lambda_handler
.
I made it work, I had to put the instrument method after the lambda function. |
And does it do something more useful than |
be098ce
to
71e2f1d
Compare
logfire/_internal/main.py
Outdated
@@ -1451,12 +1452,31 @@ def instrument_sqlite3( | |||
}, | |||
) | |||
|
|||
def instrument_aws_lambda(self, lambda_function: Any, **kwargs: Unpack[AwsLambdaInstrumentKwargs]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be type-hinted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callable[[Any, Any], Any]
, or maybe Callable[[dict[str, Any], dict[str, Any]], Any]
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if the one on the right covers all cases...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the basic one on the left is fine. Also maybe a note in docs about this param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to mention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess just mention what they're supposed to pass in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suggest, and apply whatever you'd like to see.
The example contains the function as first parameter...
logfire/_internal/main.py
Outdated
@@ -1451,12 +1452,33 @@ def instrument_sqlite3( | |||
}, | |||
) | |||
|
|||
def instrument_aws_lambda( | |||
self, lambda_function: LambdaFunction, **kwargs: Unpack[AwsLambdaInstrumentKwargs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self, lambda_function: LambdaFunction, **kwargs: Unpack[AwsLambdaInstrumentKwargs] | |
self, lambda_function_handler: LambdaFunctionHandler, **kwargs: Unpack[AwsLambdaInstrumentKwargs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples on AWS use the lambda_function
name for the function, I prefer to use the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A function handler can be any name; however, the default name in the Lambda console is lambda_function.lambda_handler. This function handler name reflects the function name (lambda_handler) and the file where the handler code is stored (lambda_function.py).
https://docs.aws.amazon.com/lambda/latest/dg/python-handler.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, lambda_handler
, not lambda_function
. mb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything else in this PR besides this? I'll merge after fixing this otherwise.
Suggestions on how to test this?