Prometheus: Fill missing steps with null values #43622

ivanahuckova · 2022-01-03T17:42:37Z

What this PR does / why we need it:

Previously (before backend migration) on frontend we filled out missing steps with null values. This PR implements this missing functionality. I've decided to implement this on backend so the results of prometheus queries are same everywhere (Dashboard, Explore, Alerting) and we don't have to rely o frontend implementation.

Which issue(s) this PR fixes:

Fixes #42993

How to test:

Run make devenv sources=prometheus
Create dashboard with query counters_logins{server="backend-01"} and time range Last 5 min and step 10s
Stop devenv_fake-prometheus-data_1 in docker and wait ~1 min. Then start it again
Where data points are missing, you should see empty/non-connected space:

On current main, the lines are connected (incorrect behaviour/bug):

ivanahuckova · 2022-01-03T17:53:48Z

pkg/tsdb/prometheus/time_series_query.go

@@ -73,8 +73,8 @@ func (s *Service) executeTimeSeriesQuery(ctx context.Context, req *backend.Query
 		timeRange := apiv1.Range{
 			Step: query.Step,
 			// Align query range to step. It rounds start and end down to a multiple of step.
-			Start: time.Unix(int64(math.Floor((float64(query.Start.Unix()+query.UtcOffsetSec)/query.Step.Seconds()))*query.Step.Seconds()-float64(query.UtcOffsetSec)), 0),
-			End:   time.Unix(int64(math.Floor((float64(query.End.Unix()+query.UtcOffsetSec)/query.Step.Seconds()))*query.Step.Seconds()-float64(query.UtcOffsetSec)), 0),
+			Start: alignTimeRange(query.Start, query.Step, query.UtcOffsetSec),


Just a refactoring. As we want to use aligning at multiple places, we created function for this.

This is an awesome improvement.

ivanahuckova · 2022-01-03T17:55:19Z

pkg/tsdb/prometheus/time_series_query.go

-		timeField := data.NewFieldFromFieldType(data.FieldTypeTime, len(v.Values))
-		valueField := data.NewFieldFromFieldType(data.FieldTypeNullableFloat64, len(v.Values))
+		baseTimestamp := alignTimeRange(query.Start, query.Step, query.UtcOffsetSec).UnixMilli()
+		datapointsCount := int((query.End.UnixMilli()-query.Start.UnixMilli())/query.Step.Milliseconds()) + 1


Number of data points that we use inNewFieldFromFieldType as we no longer can use len(v.Values).

ivanahuckova · 2022-01-03T17:56:08Z

pkg/tsdb/prometheus/time_series_query.go

-			value := float64(k.Value)
+		timeField := data.NewFieldFromFieldType(data.FieldTypeTime, datapointsCount)
+		valueField := data.NewFieldFromFieldType(data.FieldTypeNullableFloat64, datapointsCount)
+		idx := 0


Also here, we can't rely on index from for _, pair := range v.Values { and we have to keep the track ourself.

This all looks good.

Nit: Now that there's more going on, pulling this out to a function that returns timeField, valueField := ... might read a little easier.

Yeah, I totally agree. I am going to leave this for now though as @toddtreece is working on improving performance and this part of code might (probably will) change.

ivanahuckova · 2022-01-03T17:56:47Z

pkg/tsdb/prometheus/time_series_query.go

+			value := float64(pair.Value)
+
+			for t := baseTimestamp; t < timestamp; t += query.Step.Milliseconds() {
+				timeField.Set(idx, time.Unix(0, t*1000000).UTC())


If we set no value in valueField for that index, it default to nil (which is what we want).

MasslessParticle

Looks good. I think we can improve the readability just a bit.

MasslessParticle · 2022-01-03T18:12:40Z

pkg/tsdb/prometheus/time_series_query.go

@@ -73,8 +73,8 @@ func (s *Service) executeTimeSeriesQuery(ctx context.Context, req *backend.Query
 		timeRange := apiv1.Range{
 			Step: query.Step,
 			// Align query range to step. It rounds start and end down to a multiple of step.
-			Start: time.Unix(int64(math.Floor((float64(query.Start.Unix()+query.UtcOffsetSec)/query.Step.Seconds()))*query.Step.Seconds()-float64(query.UtcOffsetSec)), 0),
-			End:   time.Unix(int64(math.Floor((float64(query.End.Unix()+query.UtcOffsetSec)/query.Step.Seconds()))*query.Step.Seconds()-float64(query.UtcOffsetSec)), 0),
+			Start: alignTimeRange(query.Start, query.Step, query.UtcOffsetSec),


This is an awesome improvement.

MasslessParticle · 2022-01-03T18:13:47Z

pkg/tsdb/prometheus/time_series_query.go

-			value := float64(k.Value)
+		timeField := data.NewFieldFromFieldType(data.FieldTypeTime, datapointsCount)
+		valueField := data.NewFieldFromFieldType(data.FieldTypeNullableFloat64, datapointsCount)
+		idx := 0


This all looks good.

Nit: Now that there's more going on, pulling this out to a function that returns timeField, valueField := ... might read a little easier.

MasslessParticle · 2022-01-03T18:14:59Z

pkg/tsdb/prometheus/time_series_query_test.go

+		require.Equal(t, res[0].Fields[0].At(1), time.Unix(2, 0).UTC())
+		require.Equal(t, res[0].Fields[0].At(2), time.Unix(3, 0).UTC())
+		require.Equal(t, res[0].Fields[1].Len(), 4)
+		require.Equal(t, res[0].Fields[1].At(1), nilPointer)


Nit: These can just be require.Nil(t, ...)

pkg/tsdb/prometheus/time_series_query.go

* Prometheus: Add empty points when data points missing * Remove newline * Add comments * Improve/Fix test * Remove unused variable (cherry picked from commit d7d6c10)

* Prometheus: Add empty points when data points missing * Remove newline * Add comments * Improve/Fix test * Remove unused variable (cherry picked from commit d7d6c10) Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

PayBas · 2022-01-24T17:53:37Z

@ivanahuckova is this the new intended behavior with stacking?

Prior to 8.3.4 the stacked graph was gapless.

ivanahuckova · 2022-01-25T14:19:00Z

@PayBas yes this is a correct behaviour. Before the 8.3 there were gaps but in 8.3. we have introduced bug that removed gaps. So 8.3.4 brings it back. 🙂

PayBas · 2022-01-26T20:05:08Z

@ivanahuckova thank you for your reply. I am a little confused however. The bottom 2 images (which are the same graph/panel) shows that "Connect null values: Always" will correctly show uninterrupted lines, whereas if we also turn "Stack series" to either "Normal" or "100%", it will re-introduce gaps.

I'm concerned this PR has had an unexpected side-effect on the stacking rendering. Shouldn't stacked graphs render uninterrupted when using "Connect null values: Always"?

What I'm saying is: my panel settings were showing correctly all the way from 7.0.0 right until 8.3.3. And now since 8.3.4 I cannot get it to render gapless anymore.

ivanahuckova · 2022-01-27T13:48:16Z

@PayBas yes, might be a regression. Could you please create separate issue for this. Thank you!

Prometheus: Add empty points when data points missing

d02edf1

ivanahuckova added type/bug datasource/Prometheus add to changelog backport v8.3.x Mark PR for automatic backport to v8.3.x labels Jan 3, 2022

ivanahuckova added this to the 8.3.4 milestone Jan 3, 2022

ivanahuckova requested review from gabor, toddtreece and MasslessParticle January 3, 2022 17:42

ivanahuckova requested a review from a team as a code owner January 3, 2022 17:42

ivanahuckova requested review from papagian, kylebrandt and ying-jeanne and removed request for a team January 3, 2022 17:42

grafanabot added the area/backend label Jan 3, 2022

ivanahuckova removed request for papagian, kylebrandt and ying-jeanne January 3, 2022 17:52

ivanahuckova commented Jan 3, 2022

View reviewed changes

ivanahuckova added 2 commits January 3, 2022 18:57

Remove newline

6afb66b

Add comments

d9429e3

MasslessParticle approved these changes Jan 3, 2022

View reviewed changes

toddtreece reviewed Jan 3, 2022

View reviewed changes

pkg/tsdb/prometheus/time_series_query.go Show resolved Hide resolved

toddtreece approved these changes Jan 3, 2022

View reviewed changes

ivanahuckova added 2 commits January 4, 2022 15:21

Improve/Fix test

01d2b8d

Remove unused variable

9bf062b

ivanahuckova merged commit d7d6c10 into main Jan 5, 2022

ivanahuckova deleted the ivana/prom-add-null-for-empty branch January 5, 2022 10:40

grafanabot mentioned this pull request Jan 5, 2022

[v8.3.x] Prometheus: Fill missing steps with null values #43701

Merged

leeoniya mentioned this pull request Jan 16, 2022

'never' connect null values in time series not respected since Grafana 8.3.0 #44085

Closed

ivanahuckova mentioned this pull request Jan 24, 2022

Prometheus: Fix some of the alerting queries that use reduce/math operation #44380

Merged

vasekboch mentioned this pull request Feb 1, 2022

[regression] [8.3.3 => 8.3.4] Prometheus Metrics with gaps breaks alerting with NaN #44735

Closed

lukeyeager mentioned this pull request May 16, 2022

"null as zero" broken for prometheus queries in "Graph (old)" panels since v8.4.0 #49030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus: Fill missing steps with null values #43622

Prometheus: Fill missing steps with null values #43622

ivanahuckova commented Jan 3, 2022 •

edited

Loading

ivanahuckova Jan 3, 2022 •

edited

Loading

MasslessParticle Jan 3, 2022

ivanahuckova Jan 3, 2022

ivanahuckova Jan 3, 2022

MasslessParticle Jan 3, 2022

ivanahuckova Jan 4, 2022

ivanahuckova Jan 3, 2022

MasslessParticle left a comment

MasslessParticle Jan 3, 2022

MasslessParticle Jan 3, 2022

MasslessParticle Jan 3, 2022

PayBas commented Jan 24, 2022 •

edited

Loading

ivanahuckova commented Jan 25, 2022

PayBas commented Jan 26, 2022 •

edited

Loading

ivanahuckova commented Jan 27, 2022

Prometheus: Fill missing steps with null values #43622

Prometheus: Fill missing steps with null values #43622

Conversation

ivanahuckova commented Jan 3, 2022 • edited Loading

ivanahuckova Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MasslessParticle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PayBas commented Jan 24, 2022 • edited Loading

ivanahuckova commented Jan 25, 2022

PayBas commented Jan 26, 2022 • edited Loading

ivanahuckova commented Jan 27, 2022

ivanahuckova commented Jan 3, 2022 •

edited

Loading

ivanahuckova Jan 3, 2022 •

edited

Loading

PayBas commented Jan 24, 2022 •

edited

Loading

PayBas commented Jan 26, 2022 •

edited

Loading