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

Ensure force_flush at end of AWS Lambda invocation #296

Merged
merged 8 commits into from
Jul 5, 2024
Merged

Conversation

alexmojaki
Copy link
Contributor

No description provided.

Copy link

cloudflare-workers-and-pages bot commented Jul 1, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 45592ed
Status: ✅  Deploy successful!
Preview URL: https://ffc485b7.logfire-docs.pages.dev
Branch Preview URL: https://alex-lambda.logfire-docs.pages.dev

View logs

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@alexmojaki alexmojaki changed the title Ensure force_flush at end of AWS Lambda invocation Ensure force_flush at end of AWS Lambda invocation Jul 1, 2024
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

We should also add a config switch to not check the write token on start. Any perhaps even set it to true by default on serverless/faas platforms.

# https://github.com/getsentry/sentry-python/blob/eab218c91ae2b894df18751e347fd94972a4fe06/sentry_sdk/integrations/aws_lambda.py#L280-L314
# So we just look for the client class in all modules.
# This feels inefficient but it appears be a tiny fraction of the time `configure` takes anyway.
for mod in list(sys.modules.values()):
Copy link
Member

Choose a reason for hiding this comment

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

why do we need list here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same idea as #277. Added a comment.

@@ -797,6 +813,46 @@ def meter(self) -> metrics.Meter:
def _initialize_credentials_from_token(self, token: str) -> LogfireCredentials | None:
return LogfireCredentials.from_token(token, requests.Session(), self.base_url)

def _ensure_flush_after_lambda(self):
Copy link
Member

Choose a reason for hiding this comment

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

How slow is this in the general case?

Is there an env var we can check before looping over all modules?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _ensure_flush_after_lambda(self):
def _ensure_flush_after_aws_lambda(self):

Just to make it clear what we're talking about.

Copy link
Contributor Author

@alexmojaki alexmojaki Jul 4, 2024

Choose a reason for hiding this comment

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

How slow is this in the general case?

With this test code:

import time
import pandas
import sqlparse
import sqlalchemy
import sqlmodel
import sqlite3
import httpx
import django
import flask
import fastapi
import fastapi_cli
import uvicorn
import pydantic
import aiohttp
import pymongo
import redis

import logfire

start = time.time()
logfire.configure(
    token='foobar',
    # This speeds up `configure` by about 50% since it means it doesn't check git
    service_version='1.0.0',
)
end = time.time()
print(f'{end - start}')

Plus some tweaks in the new method, it prints:

len(list(sys.modules.values()))=1820
ensure_flush_after_lambda took 0.0005262080812826753s
0.025816917419433594

So this adds about 2% to the time that configure takes when a large number of modules have been imported already, something which will usually take much longer (>20x longer than configure in this case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an env var we can check before looping over all modules?

Right now, probably. It's very hard to trust that AWS will reliably continue to set these variables in the future.

https://github.com/search?q=repo%3Aaws%2Faws-lambda-python-runtime-interface-client%20AWS_LAMBDA_FUNCTION_NAME&type=code indicates that the lambda runtime client doesn't assume the variables are always there. And indeed they might not be if someone is running the client themselves in an emulator or something, which they encourage.

@alexmojaki
Copy link
Contributor Author

We should also add a config switch to not check the write token on start. Any perhaps even set it to true by default on serverless/faas platforms.

We already only check it in a background thread now.

@alexmojaki alexmojaki enabled auto-merge (squash) July 5, 2024 11:54
@alexmojaki alexmojaki merged commit 347f88c into main Jul 5, 2024
11 checks passed
@alexmojaki alexmojaki deleted the alex/lambda branch July 5, 2024 11:56
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.

2 participants