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

Fix: Allow null data points for time series #170

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

kevinwcyu
Copy link
Contributor

Currently, if a column includes TimeSeriesMeasureValueColumnInfo we assume that it also has a time series value

hasTimeseries = true
} else {
builders = append(builders, b)
}
}
if hasTimeseries {
// Each row is a new series
for _, timeseriesColumn := range timeseriesColumns {
for _, series := range res.Rows {
tv := series.Data[timeseriesColumn.columnIdx].TimeSeriesValue
if tv == nil {
dr.Error = fmt.Errorf("Expecting timeseries colum at: %d", timeseriesColumn.columnIdx)
return
}

However, it could just be a null data point.

This PR allows data points to be a null value instead of assuming it must be present.

This fixes #168. You can see a screenshot of what the error looked like in the panel in the linked issue.

Currently this is what the Timestream query editor shows when there are null data points.
AWS timestream query editor with null data points

With the changes in this PR this is what Grafana looks like now
Grafana timestream panel with null data points

@kevinwcyu kevinwcyu requested a review from a team as a code owner June 1, 2022 21:49
@kevinwcyu kevinwcyu requested review from asimpson and sunker and removed request for a team June 1, 2022 21:49
@github-actions
Copy link

github-actions bot commented Jun 1, 2022

Code coverage report for PR #170

Go TypeScript
main 64.8% 47.49%
PR 64.9% 47.49%
difference .1% 0%

Copy link
Collaborator

@sunker sunker left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

nice, I see you figured out how these integration tests work :)

if tv == nil {
dr.Error = fmt.Errorf("Expecting timeseries colum at: %d", timeseriesColumn.columnIdx)
isNullDataPoint := series.Data[timeseriesColumn.columnIdx].NullValue
if tv == nil && !*isNullDataPoint {
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to check if the pointer is null to avoid a possible panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oof, nice catch.

@kevinwcyu kevinwcyu changed the title Allow null data points for time series Fix: Allow null data points for time series Jun 2, 2022
@kevinwcyu kevinwcyu merged commit 4388dc5 into main Jun 2, 2022
@kevinwcyu kevinwcyu deleted the kevinwcyu/168-empty-resultset branch June 2, 2022 18:12
@brunoleite
Copy link

Grafana shows "No Data" for this scenario when using influx db instead of an empty graph. I tried to replicate the same behavior with timestream, but I did not manage to do it.

@kevinwcyu
Copy link
Contributor Author

Grafana shows "No Data" for this scenario when using influx db instead of an empty graph. I tried to replicate the same behavior with timestream, but I did not manage to do it.

@brunoleite, if you use the following

SELECT interpolatedValue
FROM interpolatedTimeseries
WHERE interpolatedValue IS NOT NULL

instead of

SELECT *
FROM interpolatedTimeseries

It should not return any rows and will show "No Data".

@brunoleite
Copy link

brunoleite commented Jun 17, 2022

Sorry for the delay. I have tried that (without this fix), and if I add WHERE interpolatedValue IS NOT NULL I only see a black graph, without the red badge.
Screen Shot 2022-06-17 at 1 21 57 PM
Some times I also see:
Screen Shot 2022-06-17 at 1 25 27 PM

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

Successfully merging this pull request may close these issues.

Empty resultset causes error on grafana
4 participants