MySQL/Postgres/MSSQL: Add parsing for day, weeks and year intervals in macros

[bernardd]

This should resolve #11431 by converting day and year intervals into hours before they're passed to time.ParseDuration. Any other intervals, including invalid ones, are passed through unchanged so that the error result remains the same.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[torkelo]

@bergquist might be relevant for prometheus, graphite & influxdb as well? maybe even the alert scheduler as well (so we can support 1d in more places)

[marefr]

As Carl wrote in #11431 (comment), there's a reason go doesn't support this. As long as it's very clear that this support is simple and don't adjust for daylight savings I guess it's a useful feature.

[bernardd]

As Carl wrote in #11431 (comment), there's a reason go doesn't support this.

Sure, but Grafana already does support it in a bunch of other places. All this is doing is bringing a few macro functions in line with the rest of the system in terms of that support.

[svenklemm]

If we define day as 24 hours we could easily support at least week here aswell and maybe even month as e.g. 31 days

[bernardd]

I'm happy to add other periods - week, month etc, if you like but initially I was just aiming for parity with the existing interval types that were used by $__interval and the auto value for Interval variables since those seemed to be the most "obvious" use cases (to my mind, at least).

[bergquist]

I agree that we need to add support for d and maybe weeks for SQL datasources since it's already supported in other areas of Grafana. I'm not sure about adding it to alerting since running alert rules every day seems like a minor edge case while rendering aggregated data per day/week is much more common.

I'm not sure how it would be implemented but I think it should be placed in another package and return a time.Duration rather than a string.

[bergquist]

I've been thinking about this and I think we should use the https://github.com/karrick/tparse library. Any chance you can update this PR to use tparse.Parse instead of NormalizeInterval ?

[bernardd]

Any chance you can update this PR to use tparse.Parse instead of NormalizeInterval ?

So I've done this, but I can't for the life of me figure out how to get the dependency auto pulled into the vendor subdirectory. This is my first time playing with either go or Grafana so if someone can point me in the right direction, I'd appreciate it.

The code itself is a little bit of a hack because tparse deals mostly in absolute times, so getting it to spit out a pure duration takes a little bit of work. It does seem to do the trick, however.

[bergquist]

@svenklemm what do you think about this?

In this solution month's depends on the length of the current month. This is far from optimal since the result when aggregating on month's period over year's will get skewed.

[svenklemm]

I think month should either calculate months properly by basing the calculation on selected timerange or be a fixed interval like 28days (for symmetry with 4 weeks) or 30 or 31 days

[marefr]

What's needed/left to do for getting this into a merge:able state?

[bernardd]

What's needed/left to do for getting this into a merge:able state?

I have no idea - happy to make any required changes if someone tells me what they are.

[SilverFire]

I would also like to see this PR merged and will help to apply the required changes.

Maybe, we can ask to review this PR once more?

[ozenati]

Is there any news about this PR?

[bergquist]

I do understand the frustration in this PR's and I've been glancing back to it from time to times but I can't get over the fact that parsing months with this solution will be problematic since it will be based on the current month. Which I dont think is a good solution. If we restricted this PR to only support days and weeks this would be an easy merge but since its support Months, I'm hesitant. While I'm sure everyone in this issue understands why aggregations might be skewed based on this I'm not sure the avg Grafana user will and their perception will be that Grafana does something wrong.

I'm not sure what to do here...

[bernardd]

If we restricted this PR to only support days and weeks this would be an easy merge but since its support Months

The only reason it supports anything other than days is that you explicitly asked me to alter it to use the tparse.Parse. It originally didn't, for the reasons I gave at #13086 (comment).

[bergquist]

I know. That's my mistake I didn't consider months when I wrote that. I'm sorry about that.
If restrict this to only support days and weeks I'm all for merging this.

If you want to do that I'm happy to make this my highest prio to review it. If not I'll fix this myself once I have spare time.

And once again sorry.

[bernardd]

I know. That's my mistake I didn't consider months when I wrote that. I'm sorry about that.

No worries.

If restrict this to only support days and weeks I'm all for merging this.

I've coded this up, but looking back at the original ticket the initial change was to include days and years, since years are also apparently potentially an auto value, so I've included those too. I don't think anyone's going to be too confused if years in this case are just 365 days - what's a quarter of a day between friends?

[bergquist]

Thank you so much for your patience! :)

LGTM