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

Lots of fixes #256

Merged
merged 39 commits into from
Aug 26, 2022
Merged

Lots of fixes #256

merged 39 commits into from
Aug 26, 2022

Conversation

TheAlienKnight
Copy link
Contributor

All changes were tested except for autosync auto deletion for templates and servers. I'll need someone to verify that works, it should though.

Changes:

  • Edited a number of queries that were being run for counts, to only attempt to fetch the estimated mongodb count, vs actually fetching the items, putting them into an array, and checking the length.
  • Changed the interval in which "daily" metrics postings occurred, to actually be a day (23.8h to be exact) and not be posting every 5 seconds. This was... causing a lot of problems
  • Changed how the site launches, it was possible for it to become "stuck" if it happened to execute quickly- it would end up waiting for the bot to emit ready. but it never got to the point where the bot could emit ready, as it held the process up.
  • If the datadog variable is an empty string, or null in the settings.json file, don't attempt to log metrics
  • Changed a few files to no longer call all databases, even when there is a switch operator to control what metrics need to be updated. Ie: Don't waste time and resources querying things that aren't being used currently
  • For automatic banlist updating, and this will only impact servers with no bans, or development testing- if there are no bans to be found, don't attempt to set a null value in redis
  • For express, the cookie-session module wants an array of strings now, even though it isn't mentioned in the docs, I tried it a few times. I just tossed the settings string into an array to make it happy.

TLDR; The site shouldn't hang and die anymore, at least it didn't in my testing. I also am assuming any memory leak that did exist, has been fixed with these changes (from what I can see anyway, and the stats, this is correct).

@IceeMC
Copy link
Member

IceeMC commented Aug 8, 2022

bark

@ConnorDoesDev
Copy link

bork

@IceeMC IceeMC requested review from IceeMC and removed request for IceeMC August 8, 2022 05:05
Copy link
Member

@IceeMC IceeMC left a comment

Choose a reason for hiding this comment

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

pog

Copy link
Member

@advaith1 advaith1 left a comment

Choose a reason for hiding this comment

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

for autosync deletion can you make it only delete on the specific error codes for invalid invite/invalid template? we don't want to nuke everything on outages and stuff

@advaith1
Copy link
Member

advaith1 commented Aug 8, 2022

also awaiting getters is weird; is there realistically much of a chance that the guilds/channels arent cached during operation? did you run into that?

can we just retrieve from cache and use optional chaining while accessing, so it doesn't totally break if they arent cached?

@TheAlienKnight
Copy link
Contributor Author

also awaiting getters is weird; is there realistically much of a chance that the guilds/channels arent cached during operation? did you run into that?

I actually ran into an issue where since my test guild is small with only a few test accounts, it wasn't actually caching the members. I thought it was odd, and the only explanation I have is that perhaps discord.js doesn't attempt to cache smaller amounts of members if they aren't actively online/interacting. I changed it for that reason, I honestly agree it is weird, I wasn't expecting it to be the issue, but it ended up being one that impacted me during testing. Regardless, it doesn't hurt to have that fallback either- even if for a guild the size of DEL it doesn't happen often. There's always a better way, but I was trying to work around the existing code vs implement new methods/ways of doing things

can we just retrieve from cache and use optional chaining while accessing, so it doesn't totally break if they arent cached?

Possibly? I can mess around with it and see it, I honestly just went with what made the most immediate sense/worked to solve the issues I ran into

for autosync deletion can you make it only delete on the specific error codes for invalid invite/invalid template? we don't want to nuke everything on outages and stuff

I can yes, I didn't think about outages actually, good point/catch there

@TheAlienKnight
Copy link
Contributor Author

Also copying the summary here:

  • No longer uselessly awaiting getters (should only happen once if needed)
  • Only delete templates and servers on proper opcodes
    • Server: 10006
    • Template: 10057

Copy link
Member

@carolinaisslaying carolinaisslaying left a comment

Choose a reason for hiding this comment

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

everything else seems okay

src/Routes/autosync.ts Outdated Show resolved Hide resolved
src/Routes/autosync.ts Outdated Show resolved Hide resolved
@carolinaisslaying
Copy link
Member

just re whether your issues have been addressed advaith

@TheAlienKnight
Copy link
Contributor Author

The changes I just pushed fix the audit logs trying to use require() in ESM, it also fixes the crash that occurs when bots are offline on startup. Those are the last two things @advaith1 pointed out in the discord.

@IceeMC IceeMC merged commit 53fb984 into discordextremelist:main Aug 26, 2022
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.

5 participants