site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

`connection-check` endpoint logging errors unnecessarily

Open aaemnnosttv opened this issue 1 year ago • 1 comments

Bug Description

Our API client logs request failures automatically. Recently we added a new request for testing if the client is connected to the internet. This is expected to "fail" from time to time and isn't useful to be reported.

This is currently reporting a significant number of events and should be updated to avoid tracking for this endpoint specifically.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Requests to core/site/data/connection-check should not track API errors

Implementation Brief

  • [ ] Update assets/js/util/api.js adding a new array called excludedDataPoints to the top of the file, with the only value being the string connection-check.
    • [ ] Add a new condition || excludedDataPoints.includes( datapoint ) the first conditional in trackAPIError: https://github.com/google/site-kit-wp/blob/5d3ec528ceb637fbd296861f4ce93e2a6d1d2a46/assets/js/util/api.js#L50-L52

Test Coverage

  • [ ] Add a new test doesn't track excluded datapoints to test the new functionality.

QA Brief

Changelog entry

aaemnnosttv avatar Feb 19 '24 20:02 aaemnnosttv

IB ✔️

eugene-manuilov avatar Mar 22 '24 16:03 eugene-manuilov

QA Update ⚠️

@eugene-manuilov

To test this I compared request on both latest and dev environment.

Q-1) I noticed that once I block request multiple failed request attempt to 'Check - connection' endpoint. Is this expected ?

Dev-

image

Latest -

image

Q-2) Regarding the Google Analytics events, no events are being tracked for the failed requests to that endpoint on both the latest and development environments. I have verified this using the Google Analytics debugger. Please let me know if my understanding is incorrect.

mohitwp avatar Apr 15 '24 14:04 mohitwp

@mohitwp

Q-1) I noticed that once I block request multiple failed request attempt to 'Check - connection' endpoint. Is this expected ?

Yes, that is expected because we check whether the internet connection has restored every X seconds.

Q-2) Regarding the Google Analytics events, no events are being tracked for the failed requests to that endpoint on both the latest and development environments. I have verified this using the Google Analytics debugger. Please let me know if my understanding is incorrect.

Oh... just noticed that this blocking approach doesn't help because it generates the fetch_error error which is also ignored. I'll update QAB.

Also I have found another problem in my code that i have fixed and created a new PR for. Moving this task to the CR column for now.

eugene-manuilov avatar Apr 15 '24 15:04 eugene-manuilov

QA Update ⚠️

  • Tested on dev environment.
  • Compared endpoint on both latest and dev environment.
  • @eugene-manuilov Regarding the Google Analytics events, no events are being tracked for the failed requests to that endpoint on both the latest and development environments. I have verified this using the Google Analytics debugger. Is this correct way to verify that ?

image

image

mohitwp avatar Apr 16 '24 12:04 mohitwp

  • Regarding the Google Analytics events, no events are being tracked for the failed requests to that endpoint on both the latest and development environments. I have verified this using the Google Analytics debugger. Is this correct way to verify that ?

@mohitwp, yeah, looks like we don't track that endpoint even. That happens most likely because we skip all requests that return the fetch_error error code. I think we get the same error code when we are offline 🤔. So, feels like changes that we did in this ticket are not necessary, but it won't hurt to keep them.

Can you try to do the following though: open the SK dashboard of your site, turn off your wifi, wait X minutes and see if you start seeing that you are in the offline mode, then check whether that GA event has been tracked for the connection-check endpoint or not.

eugene-manuilov avatar Apr 17 '24 13:04 eugene-manuilov

@eugene-manuilov I verified this by turning off wifi on both latest and dev environment. No GA event has been tracked for the connection-check endpoint. So, should we keep this code?

image

mohitwp avatar Apr 17 '24 13:04 mohitwp

Thanks, @mohitwp. Yes, let's keep it anyway.

eugene-manuilov avatar Apr 17 '24 14:04 eugene-manuilov

QA Update ✅

  • Tested on dev environment.
  • Compared endpoint on both latest and dev environment.
  • Regarding the Google Analytics events, no events are being tracked for the failed requests to that endpoint on both the latest and development environments. I have verified this using the Google Analytics debugger.

image

image

mohitwp avatar Apr 17 '24 15:04 mohitwp

Aside from a real connection request failure, this can also be raised if the user's WP session expires (prompting WP sign-in modal). In this case, the connection request fails due to a capability check because the user isn't logged in rather than a real network issue, which might be the more likely culprit.

aaemnnosttv avatar Apr 18 '24 15:04 aaemnnosttv