[core] Make hdrhistogram default measurement type

[nitsanw]

This is first step to removing internal implementation which is made obsolete by HdrHistogram

[nitsanw]

The first commit and the test fixup are required for making hdrhistogram type default.
Further work is done here to cleanup some inconsistencies in reporting.
I would like to remove histogram and the double reporting measurement type and stick with time-series and hdr-histogram. I'm pretty sure TS is also redundant with the histogram file reporting.

[nitsanw]

also, generating many PRs for these changes would be pretty annoying, please let me know if above trajectory is sensible so I don't pollute good PRs with bad

[bigbes]

@nitsanw why TS are obsolete? I want to see if perfomance is degrading (increasing of latency) if DB is used for a long period of time. Can i do this with HDRHistogram?

[nitsanw]

@bigbes histograms can be persisted to log every X seconds (the status reporting interval). From the log file you can reconstruct the percentiles over any period in the benchmark. Histograms are additive and the persistence is lossless so the data is accurate down to the status reporting interval. I think this is more than what time series have on offer.

[bigbes]

@nitsanw I can't find anywhere, where i can do thing, that you've described. ("persisted to log every X seconds")
Also, i can't find any example what i should do with *.hdr files, that i get from 'hdrhistogram.fileoutput' property.

[nitsanw]

@bigbes To control the status printout interval you can use -s T where T is the number of seconds you want the interval to be.
For HdrHistogram documentations see: https://github.com/HdrHistogram/HdrHistogram
I wrote a post on usage: http://psy-lob-saw.blogspot.com/2015/02/hdrhistogram-better-latency-capture.html
See the compressed logging section at the end.

[bigbes]

@nitsanw about '-s' - it's human-readable option, it's not very "parse-friendly".
For exporting i've used JSONArrayMeasurementsExporter. Is it hard to add support of something like this for 'exporter' output?

[nitsanw]

@bigbes the -s interval is used for histogram logging, but these are separate concerns and could be done on different intervals with different options to control them.
The binary output for the histograms could be put into the exporter, but you'll have to unpack it again yourself where as the library has tools for handling it's own log format.

[nitsanw]

Hi, can you please let me know when someone will be free to look at this? feedback?

[busbey]

We're still in the midst of getting the 0.3.0 release out the door in #358 (and it seems like most community members are overloaded at that). that should wrap up shortly which will increase the rest of our bandwidth. The easiest way for folks to get more reviewer time is helping on whatever blockers remain on that.

It's much easier to review these kinds of changes when the PR succinctly states what components get impacted and what, if any, compatibility issues are introduced. Also, it's easier when commit messages include the scope of what's impacted (a quick glance makes me think this is all [core]). While I realize PRs for the individual bits is less convenient, it also massively reduces the difficulty in reviewing. Especially since one of your commits changes a default behavior, which means it'll take more scrutiny. My apologies that #245 still is open, since it's what should cover these factors.

The big unknown I see unanswered from @bigbes is documented instructions on how folks will use hdrhistogram to generate timeseries. but again at a quick glance I think this set of commits doesn't actually remove the old timeseries, so a clear statement on if this set of patches changes that would help. The larger concern is the compatibility impact on our exporter formats, since those are what should be used for follow-on analysis.

My next solid block of review time is tomorrow in the early CDT morning. This particular change set will be hard to do then as it currently stands, since it requires checking things that are not well covered by our test sets (exporter formats) and that block of time happens to be phone-based-reviews.

[nitsanw]

@busbey Thanks for your reply. I was hoping to get an ETA.
From your comments I understand you recommend the following steps:

1. Declare component effected in PR title (fixed)
2. Declare compatibility issues: this PR changes the default reporting measurement type. As such it will potentially break compatibility for scripts/tools which rely on the default setting.
3. Clarify contents( @bigbes ): This PR does NOT remove other measurement types. I have opened a separate ticket to cover that follow-on change Remove histogram/time-series measurements as they are made obsolete by hdrhistogram #375. If that change is approved I will invest the time in supporting any previous formats/functionality to minimize transition pain, including the exporters (it would be handy to have an idea of how people use them etc).

I understand the limited time you have to spend on maintaining this repo, and appreciate your efforts. The downside for me is that making dependant changes in different PRs adds unnessary overhead, while waiting for one PR to be accepted before moving on to the next breaks my flow. I'll try and work out a workflow that supports both of us(recommendations welcome, I'm not much of a git ninja).

[busbey]

FYI, I'm trying to wrap up #98 this week and then this issue will become the next focus for reviews that require a console.

[nitsanw]

Hi,
Would splitting this into several PRs help?
Thanks

[busbey]

generally speaking, smaller targeted PRs are much easier for reviewers to get through.

In the specific case of this PR, IIRC we're just waiting on someone to go through making export files and getting enough of an understanding that we can explain the impact of this change to downstream users.

Unfortunately, I've been buried under licensing blockers over on the HBase project. @bigbes or @gkamat , could one of you take care of this issue?

[busbey]

Merged into master. Sorry again for the delay @nitsanw. Please let me know if you think the summary I made on the merge commit is accurate.

[nitsanw]

Thanks, looking good

[Thomas-Yang]

Hey @nitsanw, could you explain more specifically on how to measure the latency in time series with HDRHistogram? Let's say I would like to measure the avg and 99th latency every 10 ms. I am also interested in using HDRHistogram to match up the latency information from YCSB with the system activities (such as JVM GC) so that I can reason about the latency variation. If necessary, scale out to massive nodes (also see #415 ).

[nitsanw]

@Thomas-Yang Currently the histogram file is written out on the status interval. I think the lowest interval for writing out the file is once per second. It is unusual to require a histogram (which is a summary) for such a 10ms time window, how many samples would you expect in that period? it might be easier to log out all the latencies and summarize them in a post processing step.