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

Bug #851428 reported by Nicholas Leskiw on 2011-09-15
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
whisper
Fix Committed
High
Nicholas Leskiw

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])

Changed in graphite:
importance: Undecided → High
assignee: nobody → Nicholas Leskiw (nleskiw)
Scott Smith (ohlol) wrote :

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) wrote :

OK, uploaded my branch with a fix.

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) wrote :

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

Nicholas Leskiw (nleskiw) wrote :

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) wrote :

Testing, testing :)

Scott Smith (ohlol) wrote :

Going on 2 months with this

chrismd (chrismd) wrote :

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) wrote :

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) 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

Michael Leinartas (mleinartas) wrote :

Committed in r676

Changed in graphite:
milestone: none → 1.0.0
status: New → Fix Committed
Nicholas Leskiw (nleskiw) wrote :

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

Changed in graphite:
milestone: 1.0.0 → 0.9.10
Scott Smith (ohlol) wrote :

glad to see this finally got in :)

Sidnei da Silva (sidnei) on 2012-05-08
affects: graphite → whisper
Changed in whisper:
milestone: 0.9.10 → none
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Related questions