Fix retention units (sec, min, etc.) for storage schemas (located in whisper.py) to match those accepted by Carbon and the Webapp

[Nicholas Leskiw]

In whisper.py the units available appear to be:

UnitMultipliers = {
  's' : 1,
  'm' : 60,
  'h' : 60 * 60,
  'd' : 60 * 60 * 24,
  'y' : 60 * 60 * 24 * 365,
}

These need to be fixed to match the ones accepted by the webapp and carbon.

This includes changing the detection from an index (currently string[-1]) to a regex match ([0-9])([A-Za-z])

[Scott Smith (ohlol)]

So I don't know if you've started on this or not; I've got some code for this that just needs to be cleaned up a little. I copied the getUnitString method from webapp/graphite/render/attime.py.

[Scott Smith (ohlol)]

OK, uploaded my branch with a fix.

[Nicholas Leskiw (nleskiw)]

Cool, I'll take a look this weekend.

On Fri, Sep 16, 2011 at 5:00 PM, Scott Smith <email address hidden>wrote:

> OK, uploaded my branch with a fix.
>
> ** Branch linked: lp:~ohlol/graphite/fix-storage-schema-retention-units
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/851428
>
> Title:
> Fix retention units (sec, min, etc.) for storage schemas (located in
> whisper.py) to match those accepted by Carbon and the Webapp
>
> Status in Graphite - Enterprise scalable realtime graphing:
> New
>
> Bug description:
> In whisper.py the units available appear to be:
>
> UnitMultipliers = {
> 's' : 1,
> 'm' : 60,
> 'h' : 60 * 60,
> 'd' : 60 * 60 * 24,
> 'y' : 60 * 60 * 24 * 365,
> }
>
> These need to be fixed to match the ones accepted by the webapp and
> carbon.
>
> This includes changing the detection from an index (currently
> string[-1]) to a regex match ([0-9])([A-Za-z])
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/graphite/+bug/851428/+subscriptions
>

[Scott Smith (ohlol)]

Hey Nicholas, did you get a chance to look at the branch?

[Nicholas Leskiw (nleskiw)]

Not yet, but it's on my to-do list.

My real job has just been crazy lately.

-Nick

On Mon, Sep 26, 2011 at 4:19 PM, Scott Smith <email address hidden>wrote:

> Hey Nicholas, did you get a chance to look at the branch?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/851428
>
> Title:
> Fix retention units (sec, min, etc.) for storage schemas (located in
> whisper.py) to match those accepted by Carbon and the Webapp
>
> Status in Graphite - Enterprise scalable realtime graphing:
> New
>
> Bug description:
> In whisper.py the units available appear to be:
>
> UnitMultipliers = {
> 's' : 1,
> 'm' : 60,
> 'h' : 60 * 60,
> 'd' : 60 * 60 * 24,
> 'y' : 60 * 60 * 24 * 365,
> }
>
> These need to be fixed to match the ones accepted by the webapp and
> carbon.
>
> This includes changing the detection from an index (currently
> string[-1]) to a regex match ([0-9])([A-Za-z])
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/graphite/+bug/851428/+subscriptions
>

[Scott Smith (ohlol)]

Testing, testing :)

[Scott Smith (ohlol)]

Going on 2 months with this

[chrismd (chrismd)]

Hi Scott, just looked at your branch, a couple comments.

* Please don't bring "months" back as a unit, it's not a well-defined unit of time and people always come back with questions asking why their graphs don't look like they expect when they use it. The only reason it still exists in attime.py is for backwards compatibility, I'll announce its deprecated with the next release.
* I don't see the point of the change of 's' -> 'seconds' and using a regex, it seems much more complicated than a one letter suffix.

[chrismd (chrismd)]

Oh I see, re-read the bug description. The regex is to support the long names like 'hours' instead of 'h', etc. Sorry I missed that.

[Michael Leinartas (mleinartas)]

I just merged this in with some edits:
* removed 'month' unit per chrismd's comment
* made the check a bit more strict (more strict than attime.py ATM) so that a random word like 'scumbags' doesnt map to 'seconds' and instead throws an error

I also filed https://bugs.launchpad.net/graphite/+bug/929936 to track the deprecation of 'months' from the UI

[Michael Leinartas (mleinartas)]

Committed in r676

[Nicholas Leskiw (nleskiw)]

Thanks for kicking some butt on the Graphite list, Michael! I know I appreciate it!

-Nick

On Feb 9, 2012, at 8:01 PM, Michael Leinartas <email address hidden> wrote:

> I just merged this in with some edits:
> * removed 'month' unit per chrismd's comment
> * made the check a bit more strict (more strict than attime.py ATM) so that a random word like 'scumbags' doesnt map to 'seconds' and instead throws an error
>
> I also filed https://bugs.launchpad.net/graphite/+bug/929936 to track
> the deprecation of 'months' from the UI
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/851428
>
> Title:
> Fix retention units (sec, min, etc.) for storage schemas (located in
> whisper.py) to match those accepted by Carbon and the Webapp
>
> Status in Graphite - Enterprise scalable realtime graphing:
> Fix Committed
>
> Bug description:
> In whisper.py the units available appear to be:
>
> UnitMultipliers = {
> 's' : 1,
> 'm' : 60,
> 'h' : 60 * 60,
> 'd' : 60 * 60 * 24,
> 'y' : 60 * 60 * 24 * 365,
> }
>
> These need to be fixed to match the ones accepted by the webapp and
> carbon.
>
> This includes changing the detection from an index (currently
> string[-1]) to a regex match ([0-9])([A-Za-z])
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/graphite/+bug/851428/+subscriptions

[Scott Smith (ohlol)]

glad to see this finally got in :)