Closed Bug 764260 Opened 12 years ago Closed 12 years ago

Various networking telemetry probes are defined wrongly and may be giving misleading (wrong) results

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: briansmith, Assigned: mcmanus)

References

Details

Attachments

(1 file)

+++ 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.
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.
Assignee: nobody → mcmanus
That is correct, Patrick.
Blocks: 763359
No longer depends on: 763359
Attached patch patch 0Splinter Review
Attachment #634578 - Flags: review?(bsmith)
thanks btw brian for figuring this out.. those cache results were boggling.
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.
Attachment #634578 - Flags: review?(bsmith) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/bcf784073886
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: