-
Notifications
You must be signed in to change notification settings - Fork 7
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
Misc improvements (WIP) #8
Conversation
@@ -32,16 +35,46 @@ workflows: | |||
- type_checks: | |||
requires: | |||
- deps | |||
deployment: |
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.
👍 for comments
@@ -47,6 +51,9 @@ class Config: | |||
JWT_ACCESS_TOKEN_EXPIRES = timedelta(hours=8) | |||
CAN_SEED_DB = True | |||
|
|||
NPLUSONE_LOGGER = logging.getLogger("app.nplusone") |
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.
is this enabled by default? (I'm 99% sure that observing these extra queries is adding some overhead)
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.
how much overhead do you imagine?
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.
not much, but would like us to be aware of how much extra (not needed) stuff is done. Also, we are not going to read the logs every day so it's wasted IMHO.
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.
IMO if warnings or errors are popping up we should be reading/doing something about them.
it's my belief that some simple accounting is vastly insignificant compared to say, network requests or queries or disk operations. you're right that we shouldn't pile up unneeded stuff. i just think the potential benefit of premature CPU optimization for accounting is outweighed by the potential for performance improvements finding n+1 queries, even or especially on production. they shouldn't be ignored either, and if it turns out they are giving us false positives or things we can't do anything about we can disable it.
@@ -34,6 +35,7 @@ def create_app(test_config=None) -> App: | |||
app.wsgi_app = ProxyFix(app.wsgi_app) # type: ignore | |||
configure_database(app) | |||
api.init_app(app) # flask-rest-api | |||
NPlusOne(app) |
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.
maybe only for dev
make deploy-xx
eu-central-1
region so we're using Ireland for now.I would like to port over our new improved VPC setup so we can properly isolate the database on its own private subnet with no internet access, with a bastion EC2 host. I would like to figure out a way to do this without costing us lots of money, which is currently an issue because of the NAT gateways required when you put lambda functions in a private VPC subnet.