Significant amount of traffic sent to getpocket.cdn.mozilla.net/v3/firefox/global-video-recs?consumer_key=

bhourigan commented 5 years ago

I'm a Pocket employee.

This is not an application bug but rather a report of potentially incorrect behavior observed by the administrators of the getpocket.cdn.mozilla.net endpoint.

Over the past 30 days, we've seen approximately 82,031,606,399 hits to getpocket.cdn.mozilla.net/v3/firefox/global-video-recs?consumer_key= (consumer key is empty). These requests are invalid and being answered with a 40X response. The user agent is okhttp/3.10.0.

I think the requests are generated by Fire Tv. Specifically the code here:

https://github.com/mozilla-mobile/firefox-tv/blob/a54729ad9024162b2c83e2fff2d11daf2bfd2c49/app/src/main/java/org/mozilla/tv/firefox/pocket/PocketEndpoint.kt#L24

Unfortunately, I have no way of reproducing/testing further.

liuche commented 5 years ago

Wow, 82B is a lot of hits. We released 3.0 on Oct 4 to about 20% of our users (based on device), and then 3.0.2 around Oct 31 to almost all of our users, so let me know if you're seeing number bumps at those times that would indicate it's us.

I just checked both our recent beta build and the 3.0.2 build we released, and see that they are receiving the Pocket video feed content in-app.

I'm not sure what this could be - maybe someone has forked Firefox TV and built it, but bc we're bundling the key separately they can't load it? But this would still be a ton of hits, if that's the case. People running debug builds don't have the key, and will be sending a request with no key.

@Sdaswani do you know if anyone is making automated builds from our master for testing?

We're making a new build for 3.1 release today, so I'll make sure that our release build has the pocket key and is loading the video feed properly.

liuche commented 5 years ago

I don't see anything obvious at the moment (after checking the builds that we've released) so I'm going to let this ride until triage.

liuche commented 5 years ago

We used to have a bug where if there was an error in the parsing (or there was no key), we'd retry the request, which sounds like this problem - however, we added an exponential backoff to fix this. https://github.com/mozilla-mobile/firefox-tv/issues/1404

The devices that would be affected by this would be:

devs building without a key - this could be debug, testing, QA
LAT accidentally released without a Pocket key
people forking the repo and building
unofficial distros that people make for Android TV (because we don't provide signed builds with keys #240 )

Sdaswani commented 5 years ago

@liuche @Baron-Severin i would suggest we wireshark and logcat the tagged build to see how many calls are being made, i.e., do we think the bug that we fixed is the only source of the calls? If you are 100% sure that's fine, but I don't want us to miss another potential source of these calls. Given the amount it could be many code paths.

liuche commented 5 years ago

Well this is embarrassing - found the source of these calls, and this is from some bad background backoff code (we back off if a call succeeds so we don't blow away our cached results, but...not if it fails). Since these originate from debug builds without a key, aka dev builds, nearly all these calls probably originate from devices in our office, as they're sleeping and retrying in the background.

This is fixed in #1404 where we add an exponential backoff, but if there are any devices with old-ish builds from 2-3 months ago, we need to uninstall them or else they'll keep spamming in the background. I'll send out an email about this.

I'll also add a new UA header in our API call with a version, so you can let us know if any of the newer versions are still exhibiting bad behavior.

For more details on verification; without a Pocket key:

I checked out a commit from before exponential backoff was landed (in #1045), removed the initial 45m refresh delay, and added a log statement in Pocket.startBackgroundUpdates, and saw spamming of log statements.
On master (which has exponential backoff) I added the same log statement and saw exponential backoff on retries. (I also filed #1515 because we don't really need to retry if we don't have a key)

Sdaswani commented 5 years ago

Glad to hear this seems to be limited to internal debug builds @liuche .

@bhourigan if you can provide any more data on the source of these calls to confirm it's a limited set of clients, that would be great.

mpurzynski commented 5 years ago

If you say "network" three times you summon the NSM power.

Do you still see a lot of queries from Mozilla offices or datacenters? The majority of that traffic comes from bughunter (and bughunter cannot be discussed here).

mpurzynski commented 5 years ago

I sent a breakdown of the traffic source IP addresses, per site, to digi. If you need more, you know where to find me.

bhourigan commented 5 years ago

I reviewed the data sent by @mpurzynski in conjunction with the CloudFront logs from November. Unfortunately, it's not limited to our offices. These invalid requests came from at least 403,323 distinct IP addresses.

mpurzynski commented 5 years ago

Can you mitigate by filtering out certain user agents?

On Nov 15, 2018, at 10:11 AM, Brian Hourigan notifications@github.com wrote:

I reviewed the data sent by @mpurzynski in conjunction with the CloudFront logs from November. Unfortunately, it's not limited to our offices. These invalid requests came from at least 403,323 distinct IP addresses.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

bhourigan commented 5 years ago

The results were filtered by URI, status, and useragent. This is the BigQuery query I used:

select count(distinct requestip) from pocket_cloudfront_logs.getpocket_cdn_201811 where uri = '/v3/firefox/global-video-recs' and status = 400 and useragent = 'okhttp/3.10.0';

mcomella commented 5 years ago

@liuche @Baron-Severin Did we write a regression test for this? Should we file a follow-up to do it?

liuche commented 5 years ago

Hmmm, that's interesting news. From the cases I listed above:

devs building without a key - this could be debug, testing, QA

I would expect this to be a few set of IP addresses, but this probably doesn't account for anywhere near that number of IP addresses. @npark-mozilla what's the scale of testing that we do with non-Pocket-key builds? Are the different test builds on different IPs? @Sdaswani do you know if there is other automated testing that is being done, from master?

LAT accidentally released without a Pocket key

This should be a pretty small number of IP addresses, though - although they are still going to be making a bajillion requests (if they have the version with the bug)

people forking the repo and building unofficial distros that people make for Android TV (because we don't provide signed builds with keys #240 )

I doubt that we are getting this many forked builds. We haven't released a 3.x release build without the Pocket key through the Amazon, so this is puzzling. Are we somehow dropping the key...?

A few more things I'll check:

2.2 builds' code, and see if we added the Pocket background code there.
look at our own telemetry to see if there is anything that stands out.

@bhourigan it'd helpful to get an idea of if there's been a dropoff of requests since around November 2, when we landed the backoff fix.

Sdaswani commented 5 years ago

@bhourigan thanks for the update on the distinct IP address - 403,323 is very much. It's a bit odd though as we have around 1MM DAU using pocket on FFTV. https://sql.telemetry.mozilla.org/queries/59642#154251

Maybe the number is smaller due to cable provider IP address pooling.

@liuche is there any code path where we are mistakenly not putting in the consumer_key?

nojunpark commented 5 years ago

@liuche since we don't run automated tests on CI, non-pocket key build for testing is very minimal, unless there are many contributors downloading and building them locally to test it out.

severinrudie commented 5 years ago

@liuche @Baron-Severin Did we write a regression test for this? Should we file a follow-up to do it?

No, that seems like it should be a high priority though.

@mcomella also mentioned the possibility of writing a request interceptor that will throw a crash if too many network requests go out in a short period, which I think is a good idea.

Sdaswani commented 5 years ago

Hmmm do we want users crashing in the field for extraneous network request logic we got wrong? Can we just get the network code right and tested?

st3fan commented 5 years ago

Just to be sure, the requestip field in that database is not in the form of ip:port right?

bhourigan commented 5 years ago

Just to be sure, the requestip field in that database is not in the form of ip:port right?

Correct.

severinrudie commented 5 years ago

Hmmm do we want users crashing in the field for extraneous network request logic we got wrong? Can we just get the network code right and tested?

The code should be right and should be tested, but as the old adage goes, tests can verify bugs, they can't verify correctness. Crashing might be extreme, but at least logging to Sentry and shorting out the additional requests seems like an appropriate fallback to me.

bhourigan commented 5 years ago

@bhourigan it'd helpful to get an idea of if there's been a dropoff of requests since around November 2, when we landed the backoff fix.

No significant deviation between November 1st, 2nd, and 3rd.

habib commented 5 years ago

Folks, this bug is costing us quite a bit of $ and we'd like to see this get addressed as quickly as possible.

Sdaswani commented 5 years ago

@habib we are hoping to get a fixed release pushed next week - is that fast enough?

habib commented 5 years ago

Sounds good! Please let me know so I can engage with the vendor after the fix has gone through.

mpurzynski commented 5 years ago

Are we gonna be billed even if we filter out rouge requests, based on the uri or UA?

I can think of a couple of things that could be done.

On Nov 16, 2018, at 12:29 PM, M. Habibullah Pagarkar notifications@github.com wrote:

Sounds good! Please let me know so I can engage with the vendor after the fix has gone through.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

liuche commented 5 years ago

I've added a new UA for these requests, which will be of the form "FirefoxTV-<version>", which should roll out to all our releases devices once 3.1 is shipped. 3.1 includes a fix for backing off correctly.

I split off a separate bug #1523 to figure out where we are dropping this key, because the numbers indicate that it must be our release population.

liuche commented 5 years ago

Actually, I'll repoen this bug, because it looks like there's still conversation here.

bhourigan commented 5 years ago

Thank you, I look forward to getting this resolved.

Are we gonna be billed even if we filter out rouge requests, based on the uri or UA? I can think of a couple of things that could be done.

There is a cost associated with each request. I'll share details with you off GitHub.

netz-filter commented 5 years ago

@ liuche In some forums will be linked to GitHub and some Android TV users may use here: https://github.com/mozilla-mobile/firefox-tv/releases/tag/v3.0.1.1

The file “app-release.apk“ Firefox TV pre-release v3.0.1.1 invites you to sideload and Pocket Feature is missing here completely. Or should pocket features be completely hidden in certain countries?

Why do not you just release a beta in the Playstore?

Maybe you should only unlock Firefox for certain devices that support the Fire TV remote in the Playstore? Finally, an info that may be interesting or helpful:

https://forums.geforce.com/default/topic/1064122/shield-tv/pairing-fire-tv-or-bt-remote-to-shield-tv-oreo/

liuche commented 5 years ago

Hi @netz-filter, thank you for those links, some very good points on this.

The file “app-release.apk“ Firefox TV pre-release v3.0.1.1 invites you to sideload and Pocket Feature is missing here completely. Or should pocket features be completely hidden in certain countries?

Since Pocket is only providing english content, we turned it off for non-en locales because of concerns that people would feel that the app was too US-centric. If we see good usage, Pocket might decide to allocate more resources to provide video content for different locales.

Why do not you just release a beta in the Playstore?

Mainly we haven't decided if we want to release to the Play store (which would involve handling a different release process as well) and we haven't tested to see how well Firefox for Fire TV works on Android TV (and we already know the remote is a problem). We do have a bug filed releasing on the Play Store in #802, so let me mention @athomasmoz to see if that's something we should prioritize for 2019.

Maybe you should only unlock Firefox for certain devices that support the Fire TV remote in the Playstore?

This could definitely be an option! One problem that we're running into is that sometimes people want to continue using the remote that came with their tv (which is very reasonable!).

Thanks again for your feedback, @netz-filter - I think they're important and I'll bring it up with our team to prioritize.

netz-filter commented 5 years ago

Pocket is also annoying for me and Pocket is always displayed in DE since publication. Only in the pre-release v3.0.1.1 is Pocket completely hidden.

Maybe then also causes the false hiding "non-en locales" your problems?

There will be A users who have locked Youtube in the router for children. or B basically do not want this content from Youtube. There should be an option to disable Pocket in Settings.

Little addition:

Latest Firefox versions from the DE Amazon Appstore: Fire TV Stick 3 4K: Firefox 2.1.2 - For Firefox 2.1.2, no Pocket is displayed. Fire TV 3 4k: Firefox 3.0.2 - For Firefox 3.0.2 Pocket is displayed.

AaronMT commented 5 years ago

@netz-filter Can you verify that this is fixed now with no new 3.1 UAs?

athomasmoz commented 5 years ago

Pocket is also annoying for me and Pocket is always displayed in DE since publication. Only in the pre-release v3.0.1.1 is Pocket completely hidden.

Yes, we hid pocket for all non-EN locales because the curated videos are all EN.

There will be A users who have locked Youtube in the router for children. or B basically do not want this content from Youtube. There should be an option to disable Pocket in Settings.

We have a design change that will allow users to remove any of the sections from the homescreen #1314 . It sounds like this is a great use case for it, so I appreciate the feedback

netz-filter commented 5 years ago

@ AaronMT

Fire TV Stick 3 4K:

Today an update with the Firefox TV Version 3.0.3 has been rolled out from the DE Amazon Appstore and Pocket is hidden.
In the Firefox TV Version 3.0.3, the app can no longer be terminated with the backspace key. Bug or feature?

Firefox TV Version 3.0.3? This is probably an error in the version number, or it was a wrong version released?

@ athomasmoz

I really like the design change!

If you can not quit Firefox with the backspace key, should a shutdown function be installed here?

mozilla-mobile / firefox-tv

Significant amount of traffic sent to getpocket.cdn.mozilla.net/v3/firefox/global-video-recs?consumer_key= #1511