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

Fix blocking last logins fetching #5880

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

PierrePlt
Copy link

@PierrePlt PierrePlt commented May 17, 2024

(Probably)Fixes #5645

The problem was that fetching the last logins source IP's country of origin can sometime take a LONG time (up to a few minutes depending on the speed of the requests to dfdata.bella.network).

When the country <-> IP relationship isn't cached, this can cause the /user page to take from a few seconds to a few minutes to load for users with a long connection history.

This was caused by a variable declared in headers.inc.php that was never used.
Removing this variable solves the issue with loading the /user page but when changing tab the user would still be confronted by a long loading time since the background request to api/v1/get/last-login would block the loading of other pages.

The second fix is to only load the last logins when the recent logins menu is loaded.

Not sure if this is the issue the original author had, but this fixes the issue discussed on #5645 on the last few days

Known issue: When IP_SHORTCOUNTRY is not cached, opening the /user page, sliding down until the recent logins section is visible, and then opening another tab needing to make an API request will confront the user with a longer than usual waiting time since the request made by the second tab will be blocked by the request to api/v1/get/last-login

@mkuron
Copy link
Member

mkuron commented May 26, 2024

Thanks. It does seem like eliminating the bella.network requests significantly speeds up the login.

@T0biii
Copy link
Contributor

T0biii commented May 27, 2024

https://dfdata.bella.network/:

[09.10.2022] Added new API endpoint only for flag lookup (/country/thomas.bella.network)

maybe we should use also use the country Endpoint, or do we need the other infos?
The Json Output of country is a bit smaller and different than for lookup

country (1,2 kB):
image

lookup (1,5 kB):
image

@PierrePlt
Copy link
Author

I tried the endpoint, it doesn't seem to work with IPv6 sadly...
Regarding querying speed, it also doesn't seem to be a lot faster than the standard endpoint, it's a shame because yes as far as I know we only need shortcountry.

@T0biii
Copy link
Contributor

T0biii commented May 28, 2024

maybe @Thomas2500 can say something about IPv6 and maybe the speed?

Thomas2500 added a commit to Thomas2500/mailcow-dockerized that referenced this pull request May 28, 2024
@Thomas2500
Copy link
Contributor

Thomas2500 commented May 28, 2024

Hello @T0biii and @PierrePlt!
Thanks for the hint. The parser in the backend had an error where an IPv6 address was interpreted as an IPv4 address and thus too strongly shortened. So every IPv6 within the address space ::0 to ::fffe:ffff:ffff was found. The error has now been fixed and IPv6 addresses should now be displayed correctly.

Regarding the speed of the API:
The old API path https://dfdata.bella.network/lookup/ uses the old method to retrieve data, which includes many additional lookups such as DNS resolution, rDNS, ASN lookup and other background checks.
The new method with https://dfdata.bella.network/country/ only resolves the country and skips all other checks. In tests, the query speed was reduced from approx. 700ms to 100ms. The change should be particularly noticeable for queries like this one. I have just created a pull request #5886.

@DerLinkman
Copy link
Member

Will fixed within next release. Thanks to @Thomas2500

@DerLinkman DerLinkman closed this Jun 5, 2024
@PierrePlt
Copy link
Author

It most likely won't be fixed in the next release.
@Thomas2500's fix allows for an increase in performance when fetching IP location.
This PR removes the unnecessary fetching of all the locations when loading the /user page.

@DerLinkman DerLinkman reopened this Jun 10, 2024
Copy link
Member

@DerLinkman DerLinkman left a comment

Choose a reason for hiding this comment

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

From my side it looks fine. Sorry for closing this to early that were two seperate things for the same topic...

@DerLinkman DerLinkman merged commit 9776849 into mailcow:staging Jun 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants