1724695 - Maybe not use nsIHttpActivityDistributor in SearchSERPTelemetry.jsm

Status: VERIFIED FIXED

(Firefox :: Search, enhancement, P2)

Description

[Kershaw Chang [:kershaw]]

Currently, an observer that watches http activities is added here. This causes a lot of activity events (every transaction would dispatch 8 events mostly) need to be dispatched from socket thread to main thread.
Can we maybe observe http-on-examine-response instead?

Comment 1

[Mark Banner (:standard8)]

It looks like we'll get the same channel passed to the function, so if http-on-examine-response is equivalent to Ci.nsIHttpActivityObserver.ACTIVITY_SUBTYPE_TRANSACTION_CLOSE, then I don't see an issue with making a change -

Comment 2

[Mark Banner (:standard8)]

QE-Verify: Once this lands, please run through the search telemetry tests especially with respect to ad clicks.

Comment 3

[James Teow [:jteow]]

Hey there Kershaw, I'm going to see if I can work on this with assistance from Mark.

The two of us went through the comments in the docs, but could you elaborate a bit on the difference between http-on-examine-response and http-on-stop-request, and if there might be anything to watch for with making that change with respects to the observation of network activity for ad clicks?

Comment 4

[Kershaw Chang [:kershaw]]

Generally, I think the biggest diffrence between http-on-examine-response and http-on-stop-request is the timing.
http-on-examine-response notification is sent only when a http request receives headers. http-on-stop-request is sent when a http request is completed.
Compared to the current code (ACTIVITY_SUBTYPE_TRANSACTION_CLOSE), I can think of more difference below:

• ACTIVITY_SUBTYPE_TRANSACTION_CLOSE
  • Observed when a http transaction is closed. This means the response of this http request is only from network.
  • Happens when we receive all data completely.
• http-on-examine-response
  • Observed when a http request got response headers. Note that the headers can be from network or from cache. If you are only interested in requests from network, you could filter requests by checking isFromCache.
  • This happens before ``ACTIVITY_SUBTYPE_TRANSACTION_CLOSEandhttp-on-stop-request`.
• http-on-stop-request
  • Observed when a http request is finished.
  • Happens around the same time as ACTIVITY_SUBTYPE_TRANSACTION_CLOSE.
  • This notification also sent in child process and also sent from a http request created by a service worker.

Comment 5

[James Teow [:jteow]]

Thank you very much for the information, Kershaw.

Comment 6

[James Teow [:jteow]]

Attachment: Bug 1724695 - Switch observer in SearchSERPTelemetry to use http-on-examine-response r?standard8

Comment 7

[James Teow [:jteow]]

Hey Kershaw, just a follow up question:

I noticed there's another topic called http-on-examine-cached-response that gets a lot of cached responses that aren't sent to http-on-examine-response. I think that would explain why during smoke testing, I wasn't able to trigger isFromCache as true on the http-on-examine-response topic when I saw certain cached events in the network: the event didn't arrive because it wasn't being sent that the topic I was subscribed to. What kind of situations would result in isFromCache being true on the http-on-examine-response topic?

Comment 8

[Mark Banner (:standard8)]

Additional clarification... would ACTIVITY_SUBTYPE_TRANSACTION_CLOSE be observed for cached responses as well? I think from what you wrote, it wouldn't report the cached situations?

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to James Teow [:jteow] from comment #7)

Hey Kershaw, just a follow up question:

I noticed there's another topic called http-on-examine-cached-response that gets a lot of cached responses that aren't sent to http-on-examine-response. I think that would explain why during smoke testing, I wasn't able to trigger isFromCache as true on the http-on-examine-response topic when I saw certain cached events in the network: the event didn't arrive because it wasn't being sent that the topic I was subscribed to. What kind of situations would result in isFromCache being true on the http-on-examine-response topic?

Sorry, what I said was wrong. So, http-on-examine-response topic is only observed when a response is coming from network.
http-on-examine-response notification is sent in nsHttpChannel::ProcessResponse and nsHttpChannel::ProcessResponse is only called when a response is from network.

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Mark Banner (:standard8) from comment #8)

Additional clarification... would ACTIVITY_SUBTYPE_TRANSACTION_CLOSE be observed for cached responses as well? I think from what you wrote, it wouldn't report the cached situations?

No, ACTIVITY_SUBTYPE_TRANSACTION_CLOSE is not observed for cached responses.
ACTIVITY_SUBTYPE_TRANSACTION_CLOSE is only sent inside a http transaction, which is only used for network response.

Comment 11

[James Teow [:jteow]]

Thanks for the clarification :kershaw

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed40d89d0cf6
Switch observer in SearchSERPTelemetry to use http-on-examine-response r=Standard8

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ed40d89d0cf6

Comment 14

[Cristian Baica [:cbaica], Release Desktop QA]

Issue is verified fixed based on the latest results of adclicks recorded across bing, DDG and google, in the latest nightly search regression run.

Comment 15

[Andrej (:aglavic)]

== Change summary for alert #33720 (as of Fri, 01 Apr 2022 16:26:07 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	tp5o	linux1804-64-shippable-qr	e10s fission stylo webrender	220.81 -> 210.27
4%	tp5o	linux1804-64-shippable-qr	e10s fission stylo webrender	218.65 -> 210.11
3%	tp5o	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	209.10 -> 203.16

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=33720