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.

Sign up for GitHub

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

Jump to bottom

Adding support for daylight savings time in timeseries queries #3494

Merged
merged 29 commits into from
Dec 12, 2023
Merged

Adding support for daylight savings time in timeseries queries #3494

merged 29 commits into from
Dec 12, 2023

Conversation

AdityaHegde
Copy link
Collaborator

@AdityaHegde AdityaHegde commented Nov 15, 2023 •
edited by egor-ryashin
Loading

  • Fix extra null row around DST
  • Fix first_day_of_week giving incorrect results in the duckdb query. (Off by one day every other row) (@AdityaHegde )
  • Analyze performance regression (Egor)

@nishantmonu51 nishantmonu51 added the Type:Bug Something isn't working label Nov 15, 2023
@nishantmonu51 nishantmonu51 marked this pull request as draft November 16, 2023 09:42
@djbarnwal djbarnwal removed their request for review November 23, 2023 10:46
@AndrewRTsao AndrewRTsao mentioned this pull request Nov 27, 2023
Open
@AdityaHegde AdityaHegde self-assigned this Nov 28, 2023
@AdityaHegde AdityaHegde marked this pull request as ready for review November 28, 2023 12:09
begelundmuller
begelundmuller reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this PR look good to me.

For context for others, we found that just using time_bucket and passing a time zone to it corrects for offsets, but still treats DST changes as separate bins (with the same labels), which is the behavior we decided we want (see Notion).

Two questions before this is ready to merge:

  1. @AdityaHegde, did you confirm this does not null-fill at DST changes (i.e. just skips the value instead)?
  2. @egor-ryashin, I did some testing and the Druid implementation (not changed in this PR) appears to already have the desired behavior. Is that also your impression?

@egor-ryashin
Copy link
Contributor

Yes, it works for both.

@begelundmuller
Copy link
Contributor

Update from @AdityaHegde:

Null filling is failing with first_of_week=7. The issue is duration.Add is not honouring DST for week grain. But seem to do it for day and hour. Checking in depth

@AdityaHegde
Copy link
Collaborator Author

I have fixed the duckdb query. Interval arithmetic didnt honour time zone. Looks like it is already working in druid.

The UI test failure seem to be unrelated. It is testing formatting in leaderboards, this only changes timeseries.

@AndrewRTsao
Copy link
Contributor

As an extra validation, I went ahead and did some further testing locally. Can confirm this should fix the customer reported issues as well. However, this fix won't address #3560 so we'll want to fix that separately.

@AdityaHegde
Copy link
Collaborator Author

Note that #3560 is fixed from the API side in a previous commit. Had a debug session with @AndrewRTsao , the issue is in the formatter we use in the UI. Formatting seem to convert the UTC into another UTC with an offset leading to issues based on user's time zone. We should instead use luxon to format it and keep the underlying UTC time the same.

Lets tackle the labelling issue separately. I have reverted the labelling changes in charts from this PR so that we can fix it together.

begelundmuller
begelundmuller requested changes Dec 6, 2023
View reviewed changes
runtime/queries/metricsview_api_benchmark_test.go Outdated
@@ -114,3 +148,16 @@ func prepareEnvironment(b *testing.B) (*runtime.Runtime, string, *runtimev1.Metr
mv := obj.GetMetricsView().Spec
return rt, instanceID, mv
}

func prepareEnvironmentSpending(b *testing.B) (*runtime.Runtime, string, *runtimev1.MetricsViewSpec) {
rt, instanceID := testruntime.NewInstanceForProject(b, "spending")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not seeing this project anywhere?

runtime/queries/metricsview_timeseries.go Outdated Show resolved Hide resolved
@begelundmuller begelundmuller added the blocker A release blocker issue that should be resolved before a new release label Dec 7, 2023
@begelundmuller
Copy link
Contributor

begelundmuller commented Dec 8, 2023 •
edited
Loading

Hey @egor-ryashin, some more questions about this PR:

  1. The issue says "Fix first_day_of_week giving incorrect results" is still unresolved. Is that correct? If yes, we should fix it before merging.
  2. There's an unresolved comment about the benchmarks referencing spending project that's not committed. If you can't find another approach, can you at least add comments to the benchmark code about how to reproduce that so others can run the benchmarks?
  3. I'm still concerned that the timezone(date_trunc(timezone... trick has some other issues that we're not aware of – otherwise, time_bucket should be able to match its performance for daily granularities. Did you investigate/ask about potential issues with this trick?

@egor-ryashin
Copy link
Contributor

  1. Looking into that.
  2. Yes, but I have no definite confirmation/negation about correctness right now.

@egor-ryashin
Copy link
Contributor

  1. It's resolved.

@egor-ryashin egor-ryashin merged commit edd0ac7 into main Dec 12, 2023
4 checks passed
@egor-ryashin egor-ryashin deleted the timeseries-dst-support branch December 12, 2023 13:50
mindspank pushed a commit that referenced this pull request Dec 18, 2023
@AdityaHegde @egor-ryashin
@rakeshsharma14317 @mindspank
Adding support for daylight savings time in timeseries queries (#3494)
767db0d 
* Adding support for DST in timeseries queries



---------

Co-authored-by: egor-ryashin <[email protected]>
Co-authored-by: Egor Ryashin <[email protected]>
Co-authored-by: Rakesh Sharma <[email protected]>
esevastyanov pushed a commit that referenced this pull request Dec 19, 2023
@AdityaHegde @esevastyanov
Adding support for daylight savings time in timeseries queries (#3494)
0471d72 
* Adding support for DST in timeseries queries

---------

Co-authored-by: egor-ryashin <[email protected]>
Co-authored-by: Egor Ryashin <[email protected]>
Co-authored-by: Rakesh Sharma <[email protected]>
(cherry picked from commit edd0ac7)
esevastyanov pushed a commit that referenced this pull request Dec 19, 2023
@AdityaHegde @esevastyanov
Adding support for daylight savings time in timeseries queries (#3494)
7d9721d 
* Adding support for DST in timeseries queries

---------

Co-authored-by: egor-ryashin <[email protected]>
Co-authored-by: Egor Ryashin <[email protected]>
Co-authored-by: Rakesh Sharma <[email protected]>
(cherry picked from commit edd0ac7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@begelundmuller begelundmuller begelundmuller approved these changes

@egor-ryashin egor-ryashin egor-ryashin approved these changes

Assignees

@AdityaHegde AdityaHegde

Labels
blocker A release blocker issue that should be resolved before a new release Type:Bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AdityaHegde @egor-ryashin @begelundmuller @AndrewRTsao @nishantmonu51 @rakeshsharma14317