1037184 - Telemetry data for speculative connections should be specific to Predictor

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Jeremy Poulin [:jaypoulz]]

The telemetry data we've been gathering does not provide us with the information that we want - namely how effect are the Predictor's speculative connections. We should refactor this to be more Predictor specific.

Comment 1

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1037184-fix.patch

*** A Few Notes ***

-- Data From Last Test --

Total Predictor Preconnects = 168
Total Sockets Actually Created For These = 60
Total Sockets Create That Were Used = 1
Total Sockets Closed Without Being Used = 59

Total Number of Speculative Connections Used = 353

-- Conclusions --

Who is creating all these spec connections if not us? (nsIOService?)
Do their spec connections take precedence over ours?
Is there a race condiction here?

*** Try build ***

https://tbpl.mozilla.org/?tree=Try&rev=d2d90c4d05ad

Comment 2

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1037184-fix.patch

The last patch made try upset...
https://tbpl.mozilla.org/?tree=Try&rev=75de66b5692a

Comment 3

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1037184-fix.patch

Third time is the charm, right?
https://tbpl.mozilla.org/?tree=Try&rev=c7cfb665e7cc

Comment 4

[u408661]

Comment on attachment 8454134 [details] [diff] [review]
bug-1037184-fix.patch

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

::: netwerk/base/public/nsISpeculativeConnect.idl
@@ +61,5 @@
> +    /*
> +     * Used by the Predictor to gather telemetry data on speculative connection
> +     * usage.
> +     */
> +    readonly attribute boolean isFromPredictor;

make this infallible, as well

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +344,5 @@
>  {
>      virtual ~SpeculativeConnectArgs() {}
>  
>  public:
> +    SpeculativeConnectArgs() { mOverridesOK = mIsFromPredictor = false; }

I'm not a fan of the chained assignment. See below for the fix :)

@@ +2067,5 @@
> +
> +        if (isFromPredictor) {
> +          sock->SetIsFromPredictor(true);
> +          Telemetry::AutoCounter<Telemetry::PREDICTOR_TOTAL_PRECONNECTS_CREATED> totalPreconnectsCreated;
> +          ++totalPreconnectsCreated;

Let's move the actual telemetry accumulation to where the predictor actually creates the connection, instead of keeping it in the conn mgr.

We may also want to keep the total speculative connections telemetry, in addition to the seer-specific telemetry. Patrick, what do you think?

@@ +2796,5 @@
>          ((ignoreIdle && (ent->mIdleConns.Length() < parallelSpeculativeConnectLimit)) ||
>           !ent->mIdleConns.Length()) &&
>          !RestrictConnections(ent, ignorePossibleSpdyConnections) &&
>          !AtActiveConnectionLimit(ent, args->mTrans->Caps())) {
> +        CreateTransport(ent, args->mTrans, args->mTrans->Caps(), true, args->mIsFromPredictor);

Instead of just using args->mIsFromPredictor here, use a local value that only gets set if mOverridesOK == true (just like ignoreIdle and friends above). That way we can avoid that ugly chained assignment I complained about above :)

Comment 5

[u408661]

(In reply to Nicholas Hurley [:hurley] from comment #4)
> @@ +2067,5 @@
> > +
> > +        if (isFromPredictor) {
> > +          sock->SetIsFromPredictor(true);
> > +          Telemetry::AutoCounter<Telemetry::PREDICTOR_TOTAL_PRECONNECTS_CREATED> totalPreconnectsCreated;
> > +          ++totalPreconnectsCreated;
> 
> Let's move the actual telemetry accumulation to where the predictor actually
> creates the connection, instead of keeping it in the conn mgr.
> 
> We may also want to keep the total speculative connections telemetry, in
> addition to the seer-specific telemetry. Patrick, what do you think?

^^^^^

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #5)
> (In reply to Nicholas Hurley [:hurley] from comment #4)
> > @@ +2067,5 @@
> > > +
> > > +        if (isFromPredictor) {
> > > +          sock->SetIsFromPredictor(true);
> > > +          Telemetry::AutoCounter<Telemetry::PREDICTOR_TOTAL_PRECONNECTS_CREATED> totalPreconnectsCreated;
> > > +          ++totalPreconnectsCreated;
> > 
> > Let's move the actual telemetry accumulation to where the predictor actually
> > creates the connection, instead of keeping it in the conn mgr.
> > 
> > We may also want to keep the total speculative connections telemetry, in
> > addition to the seer-specific telemetry. Patrick, what do you think?
> 
> ^^^^^

yes - I agree with nick.

Comment 7

[Jeremy Poulin [:jaypoulz]]

I don't understand how adding telemetry into where the seer 'creates the connection' would be helpful. In my understand the seer doesn't actually create a connection - it calls spec connect which *might* create a connection. Hence why the telemetry needed to be collected in nsHttpConnMgr - this is were the connections are actually created (there a various reasons why it wouldn't create a connection, cheif among them we've already reached our maximum number of speculative connections/connections to that host). I'm fine with keeping the speculative connection telemetry, but I don't see how we can move the seer creation telemetry.

The only time where the seer actually creates a connection is in Prefetch, which as far as the world is concerned doesn't exist yet.

Comment 8

[u408661]

As discussed on irc... yes, you are right about the placement of the telemetry accumulation.

Comment 9

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1037184-fix.patch

https://tbpl.mozilla.org/?tree=Try&rev=1d0e8178e826

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aca7944ee3df

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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