Skip to content

Conversation

@sid6mathur
Copy link
Contributor

As discussed in issue #81 . Review welcome, and appreciated.
Also, it's a bit daunting to create the test setup on my local sandbox, so I will need to see the Travis bot output.

@benjaminpick
Copy link
Member

benjaminpick commented Oct 7, 2021

Exciting! I will add a few comments to you code ...

}
}
} catch (\Exception $e) {
return _geoip_detect2_get_new_empty_record($ip, $e->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Could you please check for the "HTTP/2 not supported" Error and make a more user-friendly error string out of it? "Please disable HTTP/2-support ..."
Because I think this error might still happen, e.g. when a server configuration is changed or when the site is migrated over to another server. As far as I see it, HTTP/2 is only checked when the user changes the option. Also, the detection might be incomplete and HTTP/2 doesn't work even though curl says it should work.

@benjaminpick
Copy link
Member

Oh sorry I sent you the review comments just now because I forgot to click on "submit review" ...

@benjaminpick benjaminpick changed the base branch from develop to fastah January 10, 2022 15:43
@benjaminpick
Copy link
Member

I will merge it into a feature branch and do the small changes above myself, you can continue to improve it there if you want. I would release it for 5.1.0 in some week's time.

@benjaminpick benjaminpick merged commit bd136fd into yellowtree:fastah Jan 10, 2022
@sid6mathur
Copy link
Contributor Author

Apologies, I was going to finish the requested edits this week in the new year :) . Happy to work in the feature branch too . Will support too.

@benjaminpick
Copy link
Member

The lookup is working now, but I am getting response times of 700-800ms with HTTP/2 enabled. Do you think it is because it is currently using the Wordpress implementation wp_remote_get ? I tried stream_context but it wasn't faster. Maybe we do have to implent curl_init manually?

@benjaminpick
Copy link
Member

BTW, the plugin already does client caching of the response, both in-memory and in-database (across lookups).
Most of the times, it won't be used for doing many consequent lookups of different IP adresses (because most use cases just need the IP adress of the current client), so optimizing for concurrency wouldn't speed it up here either. Hm ... Please try it out on your dev box and let me know your timings, both for servers on aws and outside (I guess your prospective clients also will be either inside or outside the AWS world, right?)

@benjaminpick
Copy link
Member

https://console.api.getfastah.com is also quite slow for me, with response times of 1-5 seconds for a page load. Other sites are fast, maybe your AWS datacenter is too far away from Germany?

@sid6mathur
Copy link
Contributor Author

sid6mathur commented Jan 11, 2022

BTW, the plugin already does client caching of the response, both in-memory and in-database (across lookups).
Most of the times, it won't be used for doing many consequent lookups of different IP adresses (because most use cases just need the IP adress of the current client), so optimizing for concurrency wouldn't speed it up here either. Hm ... Please try it out on your dev box and let me know your timings, both for servers on aws and outside (I guess your prospective clients also will be either inside or outside the AWS world, right?)

In the interest of speed of getting version 1 available to plugin users, let's remove the HTTP/2 support entirely and use the caching that you refer to as the latency-minimizer.

PS: As seen on these 2 pages, the HTTP/2 multiplexing feature needs some testing to figure out, including async use of the multi object:

PPS: Yes, the API edge is in Singapore (only) at the moment, so some latency from Germany is to be expected for the initial TCP setup. Subsequent latency depends on data caching, connection re-use, and correct enabling of HTTP/2 multiplexing.

benjaminpick added a commit that referenced this pull request Jan 26, 2022
* Fastah API datasource: initial implementation (#152)

* Initial Fastah API implementation

* Fastah API : typo fixes

* Fastah : Better copyediting

* Fastah : Better HTTP/2 capability detection and messages

* Fastah : Fix fallback HTTP proto version

* Fastah: API response parsing and data fill

* Fastah: remove wayward echo

* Fastah: fix test result expectation ordering

* add isset conditions to avoid PHP notices

* parameter english

* removing option fastah condition

* HTTP/2 support notices

* fix params

* Allow HTTP/2 support again

* try out stream_context

* error handling

* Add User Agent

* mc

Co-authored-by: Siddharth Mathur <siddharth@getfastah.com>
Co-authored-by: Benjamin Pick <benjaminpick@github.com>
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