-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: install script to migrate sentry.conf.py config to use pgbouncer #3898
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
base: master
Are you sure you want to change the base?
Conversation
This PR may only be merged after #3884 has been merged
install/migrate-pgbouncer.sh
Outdated
# Guard this file behind SETUP_PGBOUNCER_MIGRATION | ||
if [ "$SETUP_PGBOUNCER_MIGRATION" == "1" ]; then |
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 will need to remove this flag after #3884 is merged.
# Remove the file | ||
rm $SENTRY_CONFIG_PY /tmp/sentry_conf_py |
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.
Hopefully it's okay to not clean sentry/config.yml
and relay/config.yml
.
Test results are ok:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3898 +/- ##
=======================================
Coverage 99.45% 99.45%
=======================================
Files 3 3
Lines 183 183
=======================================
Hits 182 182
Misses 1 1 ☔ View full report in Codecov by Sentry. |
@aldy505 LGTM |
@@ -0,0 +1,39 @@ | |||
echo "${_group}Migrating Postgres config to PGBouncer..." |
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.
We historically have not done a sed to modify the sentry.conf.py
files, as we chose not to modify people's config files without permission.
I wonder if this is something that we should have a very loud announcement about, and keep it at that and have users modify their config files themselves.
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.
Often times there are GitHub issues complaining why certain things are broken, and the problem is that they didn't change the config file. I believe it's fine for this one, since we're trying to make everyone's instance better.
But, if we're still not wanting to go down this route, I'm fine with it.
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.
@BYK what do you think here?
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.
@hubertdeng123 I'm the one who pushed for this migration as I think it will make things quite a bit better. We do a grep-based check with a warning for memcached backend here:
echo "PyMemcacheCache found in $SENTRY_CONFIG_PY, gonna assume you're good." |
My argument for automatically doing this is:
- If you are running a custom/complex setup, you probably are not using the install script anyway
- If you are running the default setup, then we not only will be helping your setup transparently, we would also avoid running
pgbouncer
in the background for no good reason.
So if we don't want this auto migration then we should:
- Hide the
pgbouncer
container and relevant stuff behind a (possibly new) docker compose profile - We should at least add the
grep
-based check and warning we have memached and steer people in the right direction.
Which approach we take depends on our risk apetite and I cannot really force either way as I won't be the one who will be bearing the consequences 😅
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 vote for merging this :)
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 hear all the arguments and there is no perfect solution here, as we'd want to make this as seamless as possible for folks. However, I'd still prefer if this wasn't merged given the reasons:
- I wouldn't feel comfortable directly editing people's config files and setting precedent that it is fine to do in the future
- Higher risk, as we have mentioned above
Dev-infra talked about reworking how the sentry.conf.py
config file looks in sentry since the sentry.conf.py file contains a lot of defaults, not just custom settings. To me, it makes sense to have the defaults for self-hosted live in sentry in a separate file so we wouldn't need to do this. Then, people's custom sentry.conf.py
can override those defaults. This would pave a future where these sort of upgrades are seamless, even though this wouldn't provide relief immediately.
This would help us make the dev environment nicer and improve the self-hosted config story. Is this something you're interested in helping out with @aldy505?
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.
This would help us make the dev environment nicer and improve the self-hosted config story. Is this something you're interested in helping out with @aldy505?
I don't have much experience with Django. I will leave this for dev-infra.
This PR may only be merged after #3884 has been merged