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

feat(database monitor): handles multi-region and connection pooling #3685

Merged
merged 31 commits into from
Dec 17, 2024

Conversation

iainsproat
Copy link
Contributor

@iainsproat iainsproat commented Dec 12, 2024

Description & motivation

This addresses two features with the existing database monitor:

Changes:

  • rewritten in typescript. This allows us to utilise the common functionality for multi-regions in the shared package. It also removes one of the only Python components in this repository, ensuring consistent language.
  • Uses consistent logging as other components. Inconsistent logging in Python caused a mess in aggregated logging platforms.
  • Optionally expects the multi region config to include databaseName as a paramater. This allows it to gather data on the database size. If it does not exist, will log a warning and continue.

To-do before merge:

  • recreate validation on testing4 & testing5

Screenshots:

Validation of changes:

Screenshot 2024-12-13 at 10 42 42

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

GROUP BY status;
`
)
const remainingWebhookStatus = new Set(Array(4).keys())
Copy link
Contributor Author

@iainsproat iainsproat Dec 12, 2024

Choose a reason for hiding this comment

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

NOTE: This hard codes the domain logic that there are only 4 webhook status. If it ever changes this breaks.

GROUP BY "previewStatus";
`)

const remainingPreviewStatus = new Set(Array(4).keys())
Copy link
Contributor Author

@iainsproat iainsproat Dec 12, 2024

Choose a reason for hiding this comment

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

NOTE: This hard codes the domain logic that there are only 4 webhook status. If it ever changes this breaks.

return {
start: () => {
const intervalId = setInterval(() => {
void (async () => {
Copy link
Contributor Author

@iainsproat iainsproat Dec 12, 2024

Choose a reason for hiding this comment

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

NOTE: setInterval cannot be async, so we can't await the call. (or is there a better way of achieving this?)

register: prometheusClient.register,
collectionPeriodMilliseconds: databaseMonitorCollectionPeriodSeconds() * 1000,
config: {
getDbClients,
Copy link
Contributor Author

@iainsproat iainsproat Dec 12, 2024

Choose a reason for hiding this comment

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

Do we want to pass in getDbClients from elsewhere?
(minor change, can be in a different PR if at all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will probably adjust this if & when we add tests to better help dependency injection in test environments, but no need for now.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

@iainsproat iainsproat marked this pull request as ready for review December 13, 2024 12:26
Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

- as database monitor tries to monitor all configured databases, there may be unregistered databases which are not yet migrated and do not have the required tables
- many of the queries are multi-region and now query all database clients
Copy link
Contributor

📸 Preview service has generated an image.

alemagio
alemagio previously approved these changes Dec 16, 2024
Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

@iainsproat iainsproat merged commit 0115e65 into main Dec 17, 2024
42 of 44 checks passed
@iainsproat iainsproat deleted the iain/db-monitor-pooling branch December 17, 2024 09:36
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