Closed Bug 1470853 Opened Last year Closed Last year

TRR: add telemetry probe to count number of DOH requests per connection

Categories

(Core :: Networking: DNS, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: bagder, Assigned: bagder)

Details

(Whiteboard: [necko-triaged][trr])

Attachments

(4 files, 1 obsolete file)

No description provided.
Hi Nick,

Wanted to poke you for feedback on this. I want to count streams per connection when this connection was used for TRR. The current approach doesn't work as it never thinks the connection was used for TRR when checked in Http2Session::~Http2Session()

Do you think you can point out my mistake or suggest a better take?
Attachment #8999947 - Flags: feedback?(hurley)
Comment on attachment 8999947 [details] [diff] [review]
0001-bug-1470853-Add-Telemetry-DNS_TRR_REQUEST_PER_CONN-r.patch

Review of attachment 8999947 [details] [diff] [review]:
-----------------------------------------------------------------

So I'm fairly certain that what you're missing is copying nsHttpConnectionInfo::mTrrUsed in nsHttpConnectionInfo::Clone. If my reading and memory is right, when we create a conn entry, it Clone()s the connInfo from the transaction, and it's that clone that gets passed to the connection. So, since you don't copy mTrrUsed in Clone() the clone doesn't have that bit set, and so neither does the connection (which is where the h2 session gets its conninfo from).

Otherwise, this all looks good to me.
Attachment #8999947 - Flags: feedback?(hurley)
Thanks,

It seems however that in quite a lot of cases, and I think this goes for the TRR connection in particular, 'ci' will be a nullptr in Http2Session::~Http2Session() so it looks like it is too late to check if TRR has been used like that and if I instead try to figure it out and store the flag in the constructor, that is too early and ConnectionInfo() returns a nullptr there as well! 

I'm now adding a TRR counter to the Http2Session class and bump it in Http2Session::RegisterStreamID(). Would that be acceptable to you?
Attachment #8999947 - Attachment is obsolete: true
Attachment #9001559 - Flags: feedback?(hurley)
Patrick, any thoughts on "high" and "n_buckets" for this telemetry probe? I think we have reason to expect the maximum number to go really high. Several hundred requests will easily go off over a single connection within minutes so I would expect long-running sessions to manage a lot more.

This patch currently has a high at 10000 with 50 buckets, but I'm also not aware of the penalty for adding more buckets.
Flags: needinfo?(mcmanus)
more buckets is more data storage basically and also better granularity.

I would do a high of 5000 with 100 buckets.. anything over that is just basically ~infinite.
Flags: needinfo?(mcmanus)
Comment on attachment 9001559 [details] [diff] [review]
v2-0001-bug-1470853-Add-Telemetry-DNS_TRR_REQUEST_PER_CON.patch

Review of attachment 9001559 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!
Attachment #9001559 - Flags: feedback?(hurley) → feedback+
Count number of requests/streams per connection done when the connection
was used for TRR.

MozReview-Commit-ID: H7X06h8gVZY
Comment on attachment 9001559 [details] [diff] [review]
v2-0001-bug-1470853-Add-Telemetry-DNS_TRR_REQUEST_PER_CON.patch

Review of attachment 9001559 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +3329,5 @@
> +    "kind": "exponential",
> +    "high": 10000,
> +    "n_buckets": 50,
> +    "bug_numbers": [1470853],
> +    "description": "Number of DOH requests per connection"

Might be worth spelling this out to "DNS over HTTPS" since the description is already pretty short.
Comment on attachment 9001874 [details]
Request for data collection review: DNS_TRR_REQUEST_PER_CONN

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Daniel Stenberg.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default ON, only in pre-release channels.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent.
Attachment #9001874 - Flags: review?(francois) → review+
Comment on attachment 9001877 [details]
bug 1470853 - Add Telemetry::DNS_TRR_REQUEST_PER_CONN

Nicholas Hurley [:nwgh][:hurley] has approved the revision.
Attachment #9001877 - Flags: review+
Pushed by daniel@haxx.se:
https://hg.mozilla.org/integration/autoland/rev/89117b6b5799
Add Telemetry::DNS_TRR_REQUEST_PER_CONN r=nwgh
Nick,

I could use a little more guidance here!

When I try to get the connectioninfo from the stream this way, I get bitten when the stream is a pushed one it seems. I hit the assert in Http2PushTransactionBuffer::ConnectionInfo():

Assertion failure: mPushStream->Transaction() != this, at /home/daniel/src/mozilla-cinnabar/netwerk/protocol/http/Http2Push.cpp:367
#01: mozilla::net::Http2PushTransactionBuffer::ConnectionInfo() (/home/daniel/src/mozilla-cinnabar/netwerk/protocol/http/Http2Push.cpp:367)
#02: mozilla::net::Http2Session::RegisterStreamID(mozilla::net::Http2Stream*, unsigned int) (/home/daniel/src/mozilla-cinnabar/netwerk/protocol/http/Http2Session.cpp:409)
Flags: needinfo?(daniel) → needinfo?(hurley)
Hrm! I originally wondered if a push stream would cause issues, but I saw it returned a legit transaction, so I didn't worry about saying anything in the review (since you already checked for a null ci). Obviously I should have. I would just check if the stream is a push stream (we use |!(stream->StreamID() & 1)| since push streams have even IDs) and don't bother with getting the CI for that (you should never have a CI for a pushed stream at this point, since we haven't matched it to a request yet).

What you should also do (to get the telemetry right) is, when we match a pull stream to a push stream (https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/netwerk/protocol/http/Http2Stream.cpp#499) if the pull stream's CI has the istrr bit set, increment the counter in the session (DON'T do this in ConnectPushedStream, as that is pretty much guaranteed to be called multiple times). We never call RegisterStreamID when we match a pushed stream, so we can't just rely on that code.

Sorry... push makes things messy, and it's easy to miss these things.
Flags: needinfo?(hurley)
Count number of requests/streams per connection done when the connection
was used for TRR.

MozReview-Commit-ID: 50NVSCcd6jy
Comment on attachment 9003479 [details]
bug 1470853 - Add Telemetry::DNS_TRR_REQUEST_PER_CONN

Nicholas Hurley [:nwgh][:hurley] has approved the revision.
Attachment #9003479 - Flags: review+
Pushed by daniel@haxx.se:
https://hg.mozilla.org/integration/autoland/rev/92baee16f180
Add Telemetry::DNS_TRR_REQUEST_PER_CONN r=nwgh
https://hg.mozilla.org/mozilla-central/rev/92baee16f180
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.