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

add axios cached dns resolve to monitor #1598

Merged
merged 9 commits into from
Jun 13, 2022

Conversation

gregdev
Copy link
Contributor

@gregdev gregdev commented Apr 30, 2022

Description

Added in axios-cached-dns-resolve for monitor requests so that DNS lookups are cached.

Fixes #628

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

This is one of my monitors over 24 hours, on an Uptime Kuma instance with 170 monitors running on an AWS EC2 t3a-micro instance. I switched to this patch at around 22:00:
image

@gaby
Copy link
Contributor

gaby commented May 1, 2022

@gregdev Wow! I wonder if this is what's causing our Uptime-Kuma instance performance to be so abismal. We have over 300 monitors and recently switched from Host IP's to Hostnames with DNS. After making that change Uptime-Kuma became sluggish and sometimes the UI renders with no data for several seconds.

👍🏻 to get this merge

package.json Outdated
@@ -78,6 +79,7 @@
"command-exists": "~1.2.9",
"compare-versions": "~3.6.0",
"dayjs": "~1.10.8",
"esm": "^3.2.25",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? This package hasn't been updated since 2019

Copy link
Contributor

Choose a reason for hiding this comment

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

According to recent comments, that project was abandoned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes unfortunately this is required by the axios-cached-dns-resolve lib. It's required by knex so is already installed with Uptime Kuma.

@gregdev
Copy link
Contributor Author

gregdev commented May 1, 2022

I wonder if this is what's causing our Uptime-Kuma instance performance to be so abismal.

I didn't do any testing around frontend performance, but anecdotally my instance seems to load much quicker now.

@gregdev
Copy link
Contributor Author

gregdev commented May 1, 2022

CPU usage on my EC2 instance has gone down since making the change (its only task is to run Uptime Kuma):
image

@gaby
Copy link
Contributor

gaby commented May 1, 2022

CPU usage on my EC2 instance has gone down since making the change (its only task is to run Uptime Kuma): image

Yeah, that's the same thing happening to ours. Hopefully this gets merged.

@gaby
Copy link
Contributor

gaby commented May 1, 2022

@louislam Thoughts on this?

@louislam
Copy link
Owner

louislam commented May 3, 2022

Since esm was abandoned before Node.js 14 came out, I have no confidence in this package.

May need to know more how it works.

My guess is the library converts a ES module into a CommonJS module in runtime.

  • If yes, can we check the converted cjs source code?
  • Can it handle new syntax in Node.js 14 - 18? If not, when axios-cached-dns-resolve uses new syntax in the future, it may break Uptime Kuma.

Also I think we should run axios-cached-dns-resolve's unit test with esm module.
https://github.com/tcollinsworth/axios-cached-dns-resolve/tree/master/__tests__

@chakflying
Copy link
Collaborator

chakflying commented May 4, 2022

I tested a fork which reverts their own commit and turn it back to a node compatible ESM, and it seems to work okay. But I'm not sure if it's worth the trouble since it also works now and we are already using the esm package.

@louislam
Copy link
Owner

louislam commented May 4, 2022

I tested a fork which reverts their own commit and turn it back to a node compatible ESM, and it seems to work okay. But I'm not sure if it's worth the trouble since it also works now and we are already using the esm package.

It looks like modules management in Node.js is a lot more complex then I thought... I didn't know import() function can import some kind of ES6 modules.

@gaby
Copy link
Contributor

gaby commented May 5, 2022

ESM-Wallaby seems like the way to go.

I asked in the library repo about non-esm support, more info here: tcollinsworth/axios-cached-dns-resolve#25 (comment)

@louislam
Copy link
Owner

louislam commented May 6, 2022

After added their test code into our project. I am not sure why the auto-tests keep failing randomly.

But I also can't see such problem in my local environment.

For my experience, GitHub Actions performance is very bad sometimes, it kept failing while it was doing e2e testing for unknown reason. However, the test that I copied from axios-cached-dns-resolve doesn't seem to be that complex.

So I don't know which part of it is gone wrong.

…d-dns-resolve

# Conflicts:
#	package-lock.json
#	package.json
…e' into feature/axios-cached-dns-resolve

# Conflicts:
#	package-lock.json
#	package.json
@louislam
Copy link
Owner

After added their test code into our project. I am not sure why the auto-tests keep failing randomly.

But I also can't see such problem in my local environment.

For my experience, GitHub Actions performance is very bad sometimes, it kept failing while it was doing e2e testing for unknown reason. However, the test that I copied from axios-cached-dns-resolve doesn't seem to be that complex.

So I don't know which part of it is gone wrong.

I think it is due to the GitHub Actions performance issue, so let's merge it and test in beta. Thanks for the pr.

@louislam louislam merged commit 436bc13 into louislam:master Jun 13, 2022
louislam added a commit that referenced this pull request Jun 23, 2022
louislam added a commit that referenced this pull request Jun 23, 2022
@louislam
Copy link
Owner

Unfortunately, this pull request has been reverted. axios-cached-dns-resolve is not stable.
See #1821 and #1822

@Saibamen
Copy link
Contributor

New solution: #1853

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.

Very slow HTTPS (even to hosts in the same network)
5 participants