Cleaner/easier way for user to specify Cloudwatch metric percentiles

[feliksik]

Two changes:

• As proposed in Make sure Cloudwatch-metrics only send the *new* counter values.  #591 , prepend names with 'p'; that is: metricname_p50 instead of metricname_50.
• Make the WithPercentiles() options much nicer: WithPercentiles(0.50, 0.99) instead of WithPercentiles(cloudwatch.Percentiles{"50": 0.50, "0.99": 0.99})

sorry for not including these previously, I should have added [WIP] tag, as I intended to still agree on requirements, before merge.

[peterbourgon]

Is there a good reason to make the breaking p change besides consistency with other metrics?

[feliksik]

@peterbourgon : the reasons are 1) consistency and 2) clarity (that it is a percentile).

Whether that's good enough is arguable. I can also pull it out and just include the WithPercentiles() change, if you like.

[feliksik]

I believe there is no tag released since it was included, so I guessed it didn't matter much; that's why I asked @cam-stitt .

The extent to which you want to keep master backwards compatible is up to you.

[feliksik]

I see that I messed up the test:

--- FAIL: TestHistogram (0.23s)
	cloudwatch_test.go:182: p50: want 500.000000, have 0.000000; p90: want 532.038789, have 0.000000; p95: want 541.121341, have 0.000000; p99: want 558.158697, have 0.000000
FAIL

So I'll happily fix it, as soon as you can tell me which of the 2 changes you would accept :-) goodnight!

[seh]

• Why zero here, as opposed to len(percentiles)?
  It looks like you're betting in favor of them all being illegal.

[feliksik]

You are correct, make([]float64, 0, len(percentiles) would be more efficient, and will not have to re-allocate the slice twice.

I don't care to optimize this stuff for slices that are so short; in this slice is likely to be max 4 entries.

Actually, if there is an illegal entry, the len(percentiles) approach will never release the unused memory of the over-allocation ;-)

But I can fix it if you like.

[seh]

That you wrote a number at all made me take note.

I usually try to avoid the intermediate reallocations (and consequent copying) in loops like these by allocating beforehand the maximum that I might need. I'd rather hang on to that unused memory (assuming it's relatively small like this) than reallocate several times accumulating the values.

I don't know how often this code executes, so it may not make any difference. For your approach, though, I recommend just removing the length entirely, deleting line 65. You can pass a nil slice argument to append.

[feliksik]

Thanks for your comment. You are right, I never needed to set it to 0. It's better to delete the line, or initialize with len(). But the current line makes no sense.

I'll update this later :-) still waiting for a verdict on the 'p' prefix anyway.

[feliksik]

Hmm, thinking about it, the percentiles was already initialized by the beginning of the New() function. So not re-initializing it will add percentiles, not override the defaults. That's not good...

For context: The method is only used once for every cloudwatch instance you make, which is generally 1 per application runtime.

[seh]

You could write the following:

c.percentiles = nil

Whether that's better—assuming your desire is to start here with no room—is a matter of taste.

[feliksik]

Seems all fine with me; I will see if I can add some tests for the custom percentiles, and will change it to either nil or Len(), whichever you like.

[seh]

I prefer to use the length of the candidate sequence supplied to the filter—here, len(percentiles)—but my contribution to go-kit thus far has been negligible, so I defer to any others more familiar with conventions or preferences applied more consistently throughout the project.

[feliksik]

done!

[feliksik]

Great, will do! As long as nobody else comments, let's do as you suggest :-)

[feliksik]

@peterbourgon this may not be the nicest to hardcode; we can also make teststat.nvq() public. Do you have a preference?

[peterbourgon]

Maybe it's fine to just leave this [part of the] test out, for this particular metric?

I'm tentatively fine with the hard-coded percentiles, only because this whole package is getting a ground-up refactor in the short/medium term.

[feliksik]

Thanks for you comment @peterbourgon ; Do you mean leave out the whole added part about testing WithPercentile() ?
I think that's the only way.

We can also leave it hardcoded for now; it's all fine with me. I actually do think it's useful to test the WithPercentile(), but I'm not really attached to it.

If you have a preference, let me know; otherwise I'll leave it as-is.

[feliksik]

@peterbourgon you mentioned on slack "I rather leave it out, if you don't mind. Keep the old syntax."
Fine with me! The names are referted in the most recent commit.

[peterbourgon]

Thanks!