Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.43k stars 1.99k forks source link

Jetpack JITM: API call slow and hard to debug due to multiple hops #23565

Open gibrown opened 6 years ago

gibrown commented 6 years ago

The current implementation of the JITM in Calypso takes a round about path to get a response:

  1. Calypso runs a request to wp.com v1 api. Example https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/43590063/rest-api/?_envelope=1&path=%2Fjetpack%2Fv4%2Fjitm&query=%7B%22message_path%22%3A%22calypso%3Aposts-pages%3Aadmin_notices%22%7D&http_envelope=1
  2. v1 api redirects request to the Jetpack site rest api for jitm
  3. JP site then requests the JITM from the v2 wp.com api: https://github.com/Automattic/jetpack/blob/4dd36e11d44e0ac3b617476ac2fab50409630dd9/class.jetpack-jitm.php#L281
  4. Unroll it all and eventually return the response to the user

Three big problems here:

  1. Really slow. I am in the US and seeing a 2 second response time. If you are in Asia with an Asian server running the JP site then the full data path will be: Asia (browser) -> US (wp.com) -> Asia (JP) -> US (wp.com) -> Asia (JP) ->US (wp.com) -> Asia (browser)
  2. Developing new JITM means that I need to configure the JP site to send requests to my sandbox for testing.
  3. Should we be allowing a JP site change what messages we are displaying in Calpyso?

Chatted a bit with @withinboredom about this, and the main reason for this path is because there is a filter in Jetpack that lets the site owner disable these messages: https://github.com/Automattic/jetpack/blob/4dd36e11d44e0ac3b617476ac2fab50409630dd9/class.jetpack-jitm.php#L29

Two things on that:

I think we could even directly query the v2 api from Calypso and that would resolve most of the issue.

stale[bot] commented 5 years ago

This issue has been marked as stale and will be closed in seven days. This happened because:

You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation.

southp commented 5 years ago

@gibrown When you have some spare time, could you help verify if @taggon 's PR and the backend diff together will resolve this issue? We would be grateful :)

gibrown commented 5 years ago

The .com change looks like it doesn't apply to Jetpack sites since the api endpoint still ends up doing a remote call to the JP site. Probably better to have @withinboredom confirm that I am reading it correctly though.

taggon commented 5 years ago

@southp @gibrown The PR #36797 is designed only for enabling JITM for dotcom sites. However, the JITM API call to dotcom sites would be simpler and faster since it doesn't need to invoke remote Jetpack sites' API. For dotcom sites, the data path mentioned in the issue will be like Asia(browser) -> US(wp.com) -> Asia(browser).

I referred this issue because this was the reason for disabling JITM for dotcom sites. The PR doesn't completely resolve the problem.

gibrown commented 5 years ago

Cool ya, doesn't resolve this issue, and doesn't fix it for Atomic sites I guess.

taggon commented 5 years ago

and doesn't fix it for Atomic sites I guess.

@gibrown You're right. When I say dotcom sites, they don't include Atomic ones.

github-actions[bot] commented 3 years ago

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.

lsl commented 2 years ago

You're right. When I say dotcom sites, they don't include Atomic ones.

@taggon / @gibrown Is this strictly true? Can we mod this Jetpack_Direct_Call thing to work with atomic sites?

e.g. something like D85060-code?

gibrown commented 2 years ago

Commented on D85060. If we can do this for Atomic sites, then it should work that way for all Jetpack sites.

lsl commented 2 years ago

Thanks we can discuss here if its easier to track pings.

I think it came down to a Jetpack side filter? Looking at the code a bit there might be something to do with user id mapping and determining user roles too. WPCom sites have matching user id's and I think atomic do too (or at least that's what I'm banking on here)

gibrown commented 2 years ago

Maybe it would be good to move this into the jetpack repo of issues now if the Jetpack plugin is a part of what needs to change? I think there are ways to get around the user id problems now. Do you have any links or a re-summary of where we are now at?

lsl commented 2 years ago

D85060-code is where we're at currently. I don't really understand this setup much myself. Just know we can't really be expected to develop on this without making it easier to debug requests and test output.

pb5gDS-1DI-p2 - @lupus2k's initial exploration & (re)discovering the extra network hops.

pb5gDS-1Vj-p2 - Latest exploration, resulting in D85060-code.

github-actions[bot] commented 1 year ago

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.