Skip to content

Conversation

@marshallpierce
Copy link
Collaborator

Also add an example cli tool to make it easy to construct serialized
histograms of various flavors. Seems like it might come in handy for
someone beyond me.

Fixes #62.

Also add an example cli tool to make it easy to construct serialized
histograms of various flavors. Seems like it might come in handy for
someone beyond me.
@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

Hmm, tests/serialization.rs fails to compile: https://travis-ci.org/jonhoo/hdrsample/jobs/285870400#L810

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

Also, for the command-line tool, it'd be nice to also have an argument to turn on auto-resize.
Apart from that LGTM.

@algermissen
Copy link

algermissen commented Oct 10, 2017

Thanks for doing this so quickly!

I just tried the branch version:

  • Added impl Counter for i32 locally

  • produced binary with

    let mut buf = File::create("foo2.hist").expect("ok io");
    let nbytes = V2DeflateSerializer::new()
      .serialize(&hist, &mut buf).expect("ok");
    buf.sync_all().expect("OK");
    
  • The binary is attached: foo2.hist.zip

  • The base64 version is HISTFAAAAFl4AS2KOw5AUBREr+OJiOIVopOIiFKh0ShQ2JfCDizD3sQS3PeZTOYkM9NcdyUimwQRmfic33P9QvHgZBCMmhQLGYUiV1pKWhZf73Qw6ESt74ORnokfyTUHhg==

  • Created hgrm text file and imported that into http://hdrhistogram.github.io/HdrHistogram/plotFiles.html

Result:

Thanks a lot!

@marshallpierce
Copy link
Collaborator Author

Ah, there's a build with no default features. I'll add cfg as needed. Auto resize sounds good too.

@algermissen Glad it's working! Adding Counter for i32 shouldn't really be doing anything important, though. Negative values aren't supported anyway. I tried it with just plain old u64 and interop worked with the JS demo and some throwaway tools I made with the Java impl.

One other thing is that this is a backwards-incompatible change if people have caches of old histograms serialized with the plain deflate compression. If that is an issue for people we can resuscitate the old form to let them transcode the old form into the new form.

@@ -1,57 +1,61 @@
extern crate hdrsample;
#[cfg(feature = "serialization")]
#[cfg(test)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use all(feature = "serialization", test) here I believe

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

Ah, yes, that came as a result of #56.

For the example binary, we should use required-features instead of this nasty whole-module cfg gate.

As for backwards compatibility, I think we should make the change, and then provide a fix for those in need if complaints should arise.

Also, rename the example to `cli` and use a subcommand since I already
have other ideas for things to add.
@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

@marshallpierce if you think we might want to add other things, perhaps this should become a bin, and not just an example?

@marshallpierce
Copy link
Collaborator Author

Not a bad idea, but I think I'd like to incubate things a bit as an example before we ship it as something semi-official.

@jonhoo jonhoo merged commit d6b3384 into master Oct 10, 2017
jonhoo added a commit that referenced this pull request Oct 10, 2017
Note a slight backwards incompatibility for serialized histograms:
#63 (comment)
@marshallpierce marshallpierce deleted the serialization-compat branch November 1, 2017 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants