1505500 - SPDY_REQUEST_PER_CONN data is misleading

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P3)

Description

[u408661]

Noticed when I was looking at some telemetry for bug 1504337. Right now, we calculate the value for SPDY_REQUEST_PER_CONN based on the max stream ID we hit. Problem is, we use up 6 different stream IDs for our priority tree, and those IDs don't represent any request, though they do get counted as requests for our telemetry. There's a secondary problem that we actually count our "next" ID as a request, when it's not a request - it's just waiting to become one. So, in reality, every number we report (with potentially a few exceptions if someone has turned off the http/2 priority tree) is 7 higher than it actually is.

Here's what we should do:

1. Create a new telemetry probe, SPDY_REQUEST_PER_CONN_2, to replace the existing one. It can be exactly the same in all other ways.
2. Instead of calculating the number of requests based on mNextStreamID like we do now (see https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/netwerk/protocol/http/Http2Session.cpp#184), we should instead add a new counter that starts at 0, and is only incremented every time we successfully activate a stream (https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/netwerk/protocol/http/Http2Session.cpp#728)
3. In the same spot where we currently accumulate based on mNextStreamID, instead just accumulate based on the new counter, instead.

Comment 1

[komalbharadiya]

Hi u408661, I am Komal from Pune, India. I am novice to open source. But really interested to learn and contribute for this organization. Can I work on this issue? I need help, as to how do I go about it?
Thank you!

Comment 2

[Dragana Damjanovic [:dragana]]

(In reply to komalbharadiya from comment #1)
> Hi u408661, I am Komal from Pune, India. I am novice to open source. But
> really interested to learn and contribute for this organization. Can I work
> on this issue? I need help, as to how do I go about it?
> Thank you!

You can take this issue.

Comment 3

[komalbharadiya]

Hey Dragana, I was through the above links. But couldn't figure out what exactly is to be done. Can you he(In reply to Dragana Damjanovic [:dragana] from comment #2)
> (In reply to komalbharadiya from comment #1)
> > Hi u408661, I am Komal from Pune, India. I am novice to open source. But
> > really interested to learn and contribute for this organization. Can I work
> > on this issue? I need help, as to how do I go about it?
> > Thank you!
> 
> You can take this issue.

Hey Dragana, I went through the above links. But couldn't figure out what exactly is to be done. Can you help me please?

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to komalbharadiya from comment #3)
> Hey Dragana, I was through the above links. But couldn't figure out what
> exactly is to be done. Can you he(In reply to Dragana Damjanovic [:dragana]
> from comment #2)
> > (In reply to komalbharadiya from comment #1)
> > > Hi u408661, I am Komal from Pune, India. I am novice to open source. But
> > > really interested to learn and contribute for this organization. Can I work
> > > on this issue? I need help, as to how do I go about it?
> > > Thank you!
> > 
> > You can take this issue.
> 
> Hey Dragana, I went through the above links. But couldn't figure out what
> exactly is to be done. Can you help me please?

1) you need to change the name on SPDY_REQUEST_PER_CONN into SPDY_REQUEST_PER_CONN_2. This is define in:
https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#2403

2)Add a counter that is increased in at the end of Http2Session::TryToActivate function.

3) Change the line Telemetry::Accumulate(Telemetry::SPDY_REQUEST_PER_CONN, (mNextStreamID - 1) / 2);
to use the new counter instead of (mNextStreamID - 1) / 2.

Comment 5

[komalbharadiya]

(In reply to Dragana Damjanovic [:dragana] from comment #4)
> (In reply to komalbharadiya from comment #3)
> > Hey Dragana, I was through the above links. But couldn't figure out what
> > exactly is to be done. Can you he(In reply to Dragana Damjanovic [:dragana]
> > from comment #2)
> > > (In reply to komalbharadiya from comment #1)
> > > > Hi u408661, I am Komal from Pune, India. I am novice to open source. But
> > > > really interested to learn and contribute for this organization. Can I work
> > > > on this issue? I need help, as to how do I go about it?
> > > > Thank you!
> > > 
> > > You can take this issue.
> > 
> > Hey Dragana, I went through the above links. But couldn't figure out what
> > exactly is to be done. Can you help me please?
> 
> 1) you need to change the name on SPDY_REQUEST_PER_CONN into
> SPDY_REQUEST_PER_CONN_2. This is define in:
> https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/
> Histograms.json#2403
> 
> 2)Add a counter that is increased in at the end of
> Http2Session::TryToActivate function.
> 
> 3) Change the line Telemetry::Accumulate(Telemetry::SPDY_REQUEST_PER_CONN,
> (mNextStreamID - 1) / 2);
> to use the new counter instead of (mNextStreamID - 1) / 2.

Thanks alot!

Comment 6

[komalbharadiya]

Attachment: This is Http2Session.cpp. I have added a static variable cntActivate initialized to 0 in Http2Session.h as a counter. The other changes I have made in this file and histogram.json also.

Hi Dragana, Thank you for guiding. I have made changes,though not sure whether they are valid. Please check it. I am confused with patch submission as well.

Comment 7

[komalbharadiya]

Attachment: Bug 1505500 - Declared a static variable 'cntActivate' as a counter in Http2Session class. The counter is incremented in TryToActivate function. r?dragana.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:komalbharadiya, could you have a look please?

Comment 9

[komalbharadiya]

Sorry, which patch and what can I do about it?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c312a1ab6c5
Declared a static variable 'cntActivate' as a counter in Http2Session class. The counter is incremented in TryToActivate function. r=dragana,necko-reviewers.

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9c312a1ab6c5