Last Comment Bug 764260 - Various networking telemetry probes are defined wrongly and may be giving misleading (wrong) results
: Various networking telemetry probes are defined wrongly and may be giving mis...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 763351 764190
Blocks: 763359
  Show dependency treegraph
 
Reported: 2012-06-12 21:09 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-06-20 02:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (10.14 KB, patch)
2012-06-19 13:54 PDT, Patrick McManus [:mcmanus]
brian: review+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-12 21:09:21 PDT
+++ This bug was initially created as a clone of Bug #763351 +++

See the investigation in bug 763351 and bug 763359. I fixed the HTTP_*_DISPOSITION telemetry by creating new HTTP_*_DISPOSITION_2 probes using HISTOGRAM_ENUMERATED_VALUES.

There are two other networking telemetry probes that may need to be fixed in a similar way:

 SPDY_VERSION
 DNS_LOOKUP_METHOD

Should be a pretty simple change but I want to make sure that the people who work primarily on SPDY and DNS know exactly what change needs to be made, because it may mean that we have to start ignoring the above two probes and instead use new probes called SPDY_VERSION_2 and DNS_LOOKUP_METHOD_2 or similar.
Comment 1 Patrick McManus [:mcmanus] 2012-06-14 06:14:28 PDT
brian, if I follow correctly you're saying that SPDY_VERSION and DNS_LOOKUP_METHOD need to be converted to the new ENUMERATED_VALUES style (and their names changed).

we can do that under this bug.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-14 10:10:46 PDT
That is correct, Patrick.
Comment 3 Patrick McManus [:mcmanus] 2012-06-19 13:54:04 PDT
Created attachment 634578 [details] [diff] [review]
patch 0
Comment 4 Patrick McManus [:mcmanus] 2012-06-19 13:54:50 PDT
thanks btw brian for figuring this out.. those cache results were boggling.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-19 14:19:44 PDT
Comment on attachment 634578 [details] [diff] [review]
patch 0

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

r+

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +170,5 @@
>  HTTP_HISTOGRAMS(PAGE, "page: ")
>  HTTP_HISTOGRAMS(SUB, "subitem: ")
>  
> +// SPDY_VERSION was renamed to SPDY_VERSION2 as the old version did not use enumerated values
> +HISTOGRAM_ENUMERATED_VALUES(SPDY_VERSION2, 16, "SPDY: Protocol Version Used")

Make sure that this probe has a range that is as future-proof as you think is appropriate because we will have to rename the probe if we change the range.

@@ +211,5 @@
>  HISTOGRAM(CACHE_SERVICE_LOCK_WAIT, 1, 10000, 10000, LINEAR, "Time spent waiting on the cache service lock (ms)")
>  HISTOGRAM(CACHE_SERVICE_LOCK_WAIT_MAINTHREAD, 1, 10000, 10000, LINEAR, "Time spent waiting on the cache service lock on the main thread (ms)")
>  
> +// DNS_LOOKUP_METHOD was renamed to DNS_LOOKUP_METHOD2 as the old version did not use enumerated values
> +HISTOGRAM_ENUMERATED_VALUES(DNS_LOOKUP_METHOD2, 7, "DNS Lookup Type (hit, renewal, negative-hit, literal, overflow, network-first, network-shared)")

If we ever add more methods, which would change the range, apparently we would have to rename the probe again. So, it might be a good idea to extend the range while we are at it.
Comment 6 Patrick McManus [:mcmanus] 2012-06-19 17:30:40 PDT
(In reply to Brian Smith (:bsmith) from comment #5)
> Comment on attachment 634578 [details] [diff] [review]
> patch 0
> 
> Review of attachment 634578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+
> 
> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +170,5 @@
> >  HTTP_HISTOGRAMS(PAGE, "page: ")
> >  HTTP_HISTOGRAMS(SUB, "subitem: ")
> >  
> > +// SPDY_VERSION was renamed to SPDY_VERSION2 as the old version did not use enumerated values
> > +HISTOGRAM_ENUMERATED_VALUES(SPDY_VERSION2, 16, "SPDY: Protocol Version Used")
> 
> Make sure that this probe has a range that is as future-proof as you think
> is appropriate because we will have to rename the probe if we change the
> range.
> 

The fact that you say that about 16 versions is equal parts useful and depressing :)
Comment 7 Patrick McManus [:mcmanus] 2012-06-19 18:02:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf784073886
Comment 8 Ed Morley [:emorley] 2012-06-20 02:18:49 PDT
https://hg.mozilla.org/mozilla-central/rev/bcf784073886

Note You need to log in before you can comment on or make changes to this bug.