if you have fewer than 1000 measurements, return None (meaning "I don't know") when asked for the 99.9% percentile.

[arch_o_median]

This patch tests the behavior of get_latencies in two ways:

One:

The get_latencies method is covered.

Two:

In the case of a sample size of less than 1000 in a category get_latencies should return None as the value for each quantile in that category. If get_latencies simply reports the nth percentile ignoring small sample sizes the interpretation of identical values for 99th and 99.9th percentiles can become ambiguous. That is one cannot tell if they have the same value because there are many latencies with the same value at the `big' end of the distribution, or if there are simply few observations and the value at the same index in the list is being reported twice.

[zooko]

attachment:testgetlatencies.darcs.patch​ and/or attachment:testgetlatencies01.darcs.patch​ don't apply cleanly to trunk. arch_o_median is going to create new cleanly-applying patches for this ticket.

I'm renaming this ticket to reflect the desired (by me and arch_o_median at least) functionality.

[zooko]

Unsetting the review-needed keyword pending new cleanly-applying patches.

[arch_o_median]

The patch get_latenciesupgrade implements the test of get_latencies that fails ambiguous reporting for small sample sizes in test_storage.

[zooko]

Rather than returning None for all queries when fewer than 1000 samples have been taken, I would like for it to return answers for those queries which it can currently answer and return None for those that it can't. So if you've taken 100 samples and someone asks you for the 99th percentile, you should say "the 99'th percentile is 1234", and if they ask you for the 99.9th percentile, you should say "I do not know the 99.9th percentile".

arch_o_median: what do you think?

Anyone else reading this: does that make sense?

The current behavior is for it to estimate the pickier percentiles, for example if you have taken only one sample and ask it for the 99.9th percentile it will return that one value, but this violates one of my rules of measurement/monitoring/analysis/visualization, which is: "Never give a plausible answer when the truth is that you don't know."

Something that made me care a lot about this rule was when a monitoring system used by SimpleGeo showed me a graph of a CPU's load being flatly zero for a long time, then suddenly going crazy for a while, then gradually returning to almost zero. I spent several hours trying to diagnose ongoing problems in the complex, multi-server system and trying to understand how it could have exhibited that behavior when as far as I could tell the aberrant behavior should have begun much earlier than it had.

Finally it turned out that it had been going crazy during the initial period, but the measurement tool had been turned off, and the visualization represented that to me as a continuous 0 instead of as a don't-know, thus wasting my hours by showing me false data.

Returning your best guess for a 99.9th percentile when you don't have 1000 samples isn't as bad as returning 0 when you don't know, but I think it is better to return don't-know in this case. The argument that clinched it for me is that if it returns an answer then you can't distinguish between having 100 samples and the worst one being 1234 versus having 1000 samples and then 999th one being 1234, so there is a loss of information compared with if you return don't-know (None).

[arch_o_median]

Per Zooko's request the latest version of test_latencies/get_latencies has behavior that depends on the latency sample size.

Recapitulation:

(The Problem)

The notion of a percentile becomes ambiguous as the precision in the 'percentile' reported becomes over specific for the quantity of data provided. For example, if the size of a sample is less than 10 then the 01th percentile and the 10th percentile refer to the same index (the first) in the sorted list of samples. This matches the definition of a percentile, that is both 1 and 10 percent of the data is less than the first element, but can be misleading in interpretation. If the consumer believes that the 01th and 10th percentile should refer to different indices in the list then they will be mistaken. The intuition that different percentiles are references to different indices is reasonable and should be supported. The degree to which the percentiles _are_ distinct is a function of their precision and the size of the sample. Larger samples permit more precise percentiles to be meaningful. I use the word 'resolution' in my head when I think of this concept. Larger samples sizes permit higher 'resolution'.

(The Solution):

Indistinct percentiles are indicative of insufficient resolution for the specified percentile. 'Indistinct' can be simply defined as multiple references by different percentiles to the same index. The fix is quite simple, if percentiles are indistinct, they should return/report None instead of an index.

Caveat: It is, of course, possible to render all percentiles indistinct by specifying over-precise adjacent percentiles. This hack was created with the given percentile list in mind, that is, I am operating on the assumption that the consumer believes .99 and .999 to be different things but does not need to know whether .999 and .9999 are different quantities.

[arch_o_median]

Replying to zooko:

Rather than returning None for all queries when fewer than 1000 samples have been taken, I would like for it to return answers for those queries which it can currently answer and return None for those that it can't. So if you've taken 100 samples and someone asks you for the 99th percentile, you should say "the 99'th percentile is 1234", and if they ask you for the 99.9th percentile, you should say "I do not know the 99.9th percentile".

arch_o_median: what do you think?

Anyone else reading this: does that make sense?

The current behavior is for it to estimate the pickier percentiles, for example if you have taken only one sample and ask it for the 99.9th percentile it will return that one value, but this violates one of my rules of measurement/monitoring/analysis/visualization, which is: "Never give a plausible answer when the truth is that you don't know."

Something that made me care a lot about this rule was when a monitoring system used by SimpleGeo showed me a graph of a CPU's load being flatly zero for a long time, then suddenly going crazy for a while, then gradually returning to almost zero. I spent several hours trying to diagnose ongoing problems in the complex, multi-server system and trying to understand how it could have exhibited that behavior when as far as I could tell the aberrant behavior should have begun much earlier than it had.

Finally it turned out that it had been going crazy during the initial period, but the measurement tool had been turned off, and the visualization represented that to me as a continuous 0 instead of as a don't-know, thus wasting my hours by showing me false data.

Returning your best guess for a 99.9th percentile when you don't have 1000 samples isn't as bad as returning 0 when you don't know, but I think it is better to return don't-know in this case. The argument that clinched it for me is that if it returns an answer then you can't distinguish between having 100 samples and the worst one being 1234 versus having 1000 samples and then 999th one being 1234, so there is a loss of information compared with if you return don't-know (None).

[arch_o_median]

I strongly disapprove of 'statistics' are over-precise... NNT would agree with me I think.

[zooko]

I asked Brian on IRC if he would be okay with this change. He replied something like "Yeah, but you need to find all consumers which are using that result and change them to handle None instead of numbers in those cases."

So, I'll do that part.

Hm, reviewing the patch now -- if you have 10 samples then you could give an answer for 90th percentile, right? The highest sample would be the 90th percentile, if I understand the definition of "percentile" correctly. The current patch has None for 90th percentile when there are 10 samples. Why is that?

[zooko]

arch_o_median will do the "finding all consumers" part. There are two types of consumers:

• those that invoke the Python API, i.e. any Python code that invokes get_stats()​ or get_latencies()​
• those that use the WUI/WAPI, i.e. any HTTP clients that are reading the HTTP results rendered by web.storage.StorageStatus.render_JSON()​ or the "stats" magic Nevow variable​. Looking at stats.rst​, it appears that only code that is looking for the key latencies in the resulting stats dict could be affected. Grepping for latencies in the Tahoe-LAFS code base, I see that misc/operations_helpers/munin/tahoe_server_latency_​ is the only such in our source tree. There is documentation about the Munin plugins in stats.rst​, and also at the top of the munin/tahoe_server_latency_ file.

[arch_o_median]

OK, the latest patch _also_ has an updated docstring for get_latencies.

[zooko]

attachment:test_get_latencies_upgraded01.darcs.2.patch​ looks good to me. After you hunt down all code and tools that use these stats (i.e. the munin script) and if needed update the docs/stats.rst​ file, I'll commit the patch(es) to trunk.

[arch_o_median]

"grepped" potential consumers

[arch_o_median]

Okay heres a grepped list of "def get_stats". This lets me know what the possibilities are for a given invocation.

[arch_o_median]

A warner-compiled list of consumers in irc copy-paste format

[zooko]

attachment:test_get_latencies_upgraded01.darcs.2.patch​ and attachment:statsdoc.darcs.patch​ look good to me. I read through Brian's search for consumers in attachment:statsdoc.darcs.patch​ and agree that there are probably no consumers that will get tripped up by this change. There are two remaining items: 1. a manual test by using a server that has this patch and then looking at its /statistics page (arch_o_median plans to do this tonight), 2. add an entry to source:NEWS.rst​ so that anyone who is using the json HTTP interface to get stats will see a note saying that this has changed. To remind us of the latter, I've added the news-needed tag to this ticket.

[arch_o_median]

adds mention of percentile correction to docs/stats.rst

[arch_o_median]

Contains previous patches and a modification to interfaces to allow None to be among values in get_stats returned dict.

[arch_o_median]

When I ran the whole test suite "python ./setup.py test" two tests failed. The dictionary returned by get_stats no longer conformed to the constraints expected in interfaces.py as it contained "None" values. I modified the ChoiceOf call in the relevant interface to include a "None", element as an argument. This allows the test suite to pass. Beyond the fact that the tests now pass I do not know what effects such a change might have, i.e. I've not examined the definition of ChoiceOf, so review is certainly called for.

[arch_o_median]

Example of "operational statistics" output with "None" values. (Manual test)

[arch_o_median]

complete patch: teststorage, stats, interfaces, NEWS all modified

[arch_o_median]

The last file attached to this ticket contains what I believe to be a complete patch. Among its contents are a unit test, modified get_latencies function, modified RIStatsProvider interface, modified docs in multiple files. Also attached to this ticket is a file "statistics.html" that contains the output of "Operational Statistics" using the patched code and small numbers of operations, with the resultant "Nones".

[zooko]

attachment:patchcompletewithdocchanges.darcs.patch​ looks great!

[wilcoxjg@…]

In 67ad0175cd3e4870:

server.py: get_latencies now reports percentiles _only_ if there are sufficient observations for the interpretation of the percentile to be unambiguous.
interfaces.py: modified the return type of RIStatsProvider.get_stats to allow for None as a return value
NEWS.rst, stats.py: documentation of change to get_latencies
stats.rst: now documents percentile modification in get_latencies
test_storage.py: test_latencies now expects None in output categories that contain too few samples for the associated percentile to be unambiguously reported.
fixes #1392