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

PublicDashboards: Support timezone on query API #68560

Merged
merged 16 commits into from
Jun 14, 2023

Conversation

evictorero
Copy link
Contributor

@evictorero evictorero commented May 16, 2023

What is this feature?

  • Add the location in the request to support relative time ranges correctly.
  • Move business logic from model package to service
  • Add unit test for relative time ranges like yesterday and last 1 hour

Which issue(s) does this PR fix?:

Fixes #62903
Enterprise PR: https://github.com/grafana/grafana-enterprise/pull/5148

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@evictorero evictorero requested a review from a team as a code owner May 16, 2023 15:08
@evictorero evictorero requested review from dprokop and kaydelaney and removed request for a team May 16, 2023 15:08
@evictorero evictorero added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels May 16, 2023
@evictorero evictorero added this to the 10.1.x milestone May 16, 2023
evictorero and others added 4 commits May 17, 2023 09:51
Co-authored-by: Juan Cabanas <juan.cabanas@grafana.com>
…ce.ts

Co-authored-by: Juan Cabanas <juan.cabanas@grafana.com>
type PublicDashboardQueryDTO struct {
IntervalMs int64
MaxDataPoints int64
QueryCachingTTL int64
TimeRange TimeSettings
TimeRange TimeRangeDTO
Copy link
Member

Choose a reason for hiding this comment

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

@evictorero wonder why Location is added to TimeRangeDTO? Think we should use standard TimeRange struct rather than introducing public dashboard specific DTO. What's the issue with placing the Location on PublicDashboardQueryDTO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move Location to the upper level but I think it makes sense since it is related to the from and to.
Which standard TimeRange struct are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are talking about the TimeSettings struct that I am removing right? That is a model struct, we created it to support saving the TimeSettings data persisted for each public dashboard but we ended up not using it, there is a plan to remove it. On the other hand, we are also adding DTOs to separate the entities of the data transfer objects to remove coupling, that is why makes sense to have a DTO for the TimeRange.

Copy link
Member

Choose a reason for hiding this comment

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

@evictorero I'm talking about TimeRange strict available in github.com/grafana/grafana-plugin-sdk-go/backend . I'm not sure why we need to introduce yet another time range struct, while we have a standard one available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That TimeRange is different, the field types are Time. We have strings because we are supporting relative time ranges (like now or now-1d) in the API (even when the frontend always sends the epoch in ms). We could change it to TimeRange but we wouldn't support relative time ranges anymore.

@evictorero evictorero changed the title PublicDashboards: Support location on query API PublicDashboards: Support timezone on query API Jun 8, 2023
@evictorero evictorero added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Jun 8, 2023
@evictorero evictorero merged commit fc374f9 into main Jun 14, 2023
14 checks passed
@evictorero evictorero deleted the evictorero/public-dashboards-use-timezone-query branch June 14, 2023 20:35
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task: Receive the timezone in the Query API
5 participants