Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

toStartOfMonth() Returning Incorrect Values #295

Closed
dbernardi7 opened this issue Feb 7, 2023 · 12 comments · Fixed by #314
Closed

toStartOfMonth() Returning Incorrect Values #295

dbernardi7 opened this issue Feb 7, 2023 · 12 comments · Fixed by #314
Assignees

Comments

@dbernardi7
Copy link

What happened: When using toStartOfMonth() in a query in Grafana, the result of the query mimicked what toEndOfMonth() would produce with all the values indicating the last day of the month. However, when using the ClickHouse CLI the query returned expected beginning of the month values.

What you expected to happen: I would've expected to have the first date of the month returned with a DateTime input. For example if entering this: 2022-12-07 02:15:10, I would expect to be returned this: 2022-12-01.

How to reproduce it (as minimally and precisely as possible): While connecting Grafana to a ClickHouse database have a field that is a DateTime data type. Query the field using the function toStartOfMonth(). The result will show the last day of the month of the date that you queried.

An example query:

SELECT toStartOfMonth( "DateTime Field" )
FROM datatable

Screenshots

image

Anything else we need to know?: When using the clickhouse client to perform the same exact query, the correct result is shown. Screenshot below:

image

Environment:

  • Grafana version: v8.4.4 (fcfb01faeb)
  • Plugin version:
  • OS Grafana is installed on: NA
  • User OS & Browser: Chrome on Mac OS X 10.15
  • Others:
@dbernardi7 dbernardi7 added datasource/ClickHouse type/bug Something isn't working labels Feb 7, 2023
@jkaflik
Copy link
Collaborator

jkaflik commented Feb 13, 2023

Hi @dbernardi7,

The issue's root cause is simple, but I am unsure how we can mitigate it.
toStartOfMonth in ClickHouse return Date type. This type is decoded as a Go time.Time type in clickhouse-go library.

The problem is Date type is cast to time.Time with a local server timezone. In our case, it's Grafana server timezone.

If you go and pick in Grafana UI time picker the same timezone as you have in Grafana backend, results should be fine.

Most likely, this issue has to be fixed in clickhouse-go, so Date decode loads a user-defined timezone.

@jkaflik jkaflik self-assigned this Feb 13, 2023
@dbernardi7
Copy link
Author

Hey @jkaflik,

Thank you so much for your prompt response, I really appreciate it!

I went ahead and changed the organizations timezone to UTC and ran a test query to ensure it worked, and sure enough it did!

However, while it does work, I can't reason why that would cause the dates to appear as the end of the month as they are in the screenshots above. For example for today's date (February 13th), if it was 2pm PST, which would be 10pm UTC, the starting date of the month should still return the same value: 2023-02-13.

My point is that even if the time was hours ahead or behind UTC, but still in the same month, the function should still return the start of the month. And in the rare case that it is the last day of the month PST and in UTC it is now the first of the month, the function should still return the start of the month and not the last day.

Thank you again for your timeliness on this and all the information so far!

@jkaflik
Copy link
Collaborator

jkaflik commented Feb 14, 2023

@dbernardi7

However, while it does work, I can't reason why that would cause the dates to appear as the end of the month as they are in the screenshots above. For example for today's date (February 13th), if it was 2pm PST, which would be 10pm UTC, the starting date of the month should still return the same value: 2023-02-13.

If I understand the problem correctly, but let give me a try to explain:

Let's assume we have DateTime with any timezone: 2022-07-17 15:00:00
when we query CH with SELECT toStartOfMonth(toDateTime('2022-07-17 15:00:00')) it returns us a Date 2022-07-01 which is timezone agnostic.

Later, this value is decoded by clickhouse-go and return as DateTime with a server local timezone. For example, it's UTC 2022-07-01 00:00:00.
At the latest stage, Grafana UI loads this and shows you in your -4 timezone as 2022-06-30 20:00:00 (so you see end of the month instead :) )

The solution would be that Grafana UI sends the client timezone, which is used for CH Date decode.

Hope this answers your question.

Edit:

It would make sense to load the date with ClickHouse server timezone. Let me discuss that internally with the team.

@asimpson
Copy link
Contributor

This is a two-part fix. The first part @jkaflik has already started which is updating clickhouse-go to use a timezone in its Date parsing. The second part would be to update the datasource to send along a timezone. We can probably use existing functionality in grafana-data.

@jkaflik
Copy link
Collaborator

jkaflik commented Feb 17, 2023

@asimpson the second part - is it something you will work on?

@asimpson
Copy link
Contributor

Let me discuss with @grafana/partner-plugins and see when we can fit it in.

@mshustov
Copy link
Collaborator

mshustov commented Feb 24, 2023

The second part would be to update the datasource to send along a timezone.

@asimpson How can we attach additional metadata to a query?
DataQueryRequest interface contains timezone value (even though, by default is browser, which is not helpful for the backend), but it's not attached to a query sent by DataSourceWithBackend see the implementation https://github.com/grafana/grafana/blob/98596c36ad733d159c0539eae160d0677f97ae7a/packages/grafana-runtime/src/utils/DataSourceWithBackend.ts#L122
I don't see any mechanism to attach metadata to the query (like access to HTTP request body, URL, header, etc.).
I noticed that some plugins don't use DataSourceWithBackend.query method and deal with the HTTP interface directly (for example, Cloudwatch). I don't find this idea very appealing, since a custom query handler creates divergence from the canonical implementation in DataSourceWithBackend class.

Maybe we can consider extending the DataSourceWithBackend.query interface to allow a client-side plugin to supply additional metadata to a backend counterpart? Do you have other ideas?

@asimpson
Copy link
Contributor

You're totally correct, I was looking at Cloudwatch when I was thinking about it last week just because of the timezone piece. One thing I want to uncover is why Cloudwatch made the decision it did.

Maybe we can consider extending the DataSourceWithBackend.query interface to allow a client-side plugin to supply additional metadata to a backend counterpart? Do you have other ideas?

I think this is a good idea! I'm curious how this will work out in practice.

@mshustov
Copy link
Collaborator

mshustov commented Feb 27, 2023

I'm curious how this will work out in practice.

@asimpson what if we change the query interface to preconfigure some properties of an HTTP request?
We can start with headers or params? For example,BackendSrvRequest interface, which is used in postResource and getResource provides such an ability:
https://github.com/grafana/grafana/blob/98596c36ad733d159c0539eae160d0677f97ae7a/packages/grafana-runtime/src/services/backendSrv.ts#L8-L82

I'd suggest changing DataQueryRequest.query signature to the following:

query(
  request: DataQueryRequest<TQuery>,
  {
     headers?: ReadonlyMap<string, string>,
     params?: ReadonlyMap<string, string | number | null >,
  }
) => Observable<DataQueryResponse>

@asimpson
Copy link
Contributor

asimpson commented Mar 3, 2023

what if we change the query interface to preconfigure some properties of an HTTP request?
We can start with headers or params

I think this would work. We can always refine or refactor if we uncover a better way.

@mshustov
Copy link
Collaborator

mshustov commented Mar 3, 2023

@jkaflik could you use #313 as a foundation for adjusting the timezone in the datasource?

@jkaflik
Copy link
Collaborator

jkaflik commented Mar 3, 2023

@mshustov @asimpson, it seems we are about the resolve this fantastic issue :)
There are two PRs:
#314 (@mshustov changes + few lines from my side)
grafana/sqlds#87

I will also release clickhouse-go soon. I wanted to update go.mod to point to a specific fork/version, but I failed on this. Will follow up on it as we get everything merged & released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants