Skip to content

Comments

Fix avg_over_time for nan and float64 overflows#7346

Merged
roidelapluie merged 5 commits intoprometheus:masterfrom
roidelapluie:avg_over_time
Jul 13, 2020
Merged

Fix avg_over_time for nan and float64 overflows#7346
roidelapluie merged 5 commits intoprometheus:masterfrom
roidelapluie:avg_over_time

Conversation

@roidelapluie
Copy link
Member

@roidelapluie roidelapluie commented Jun 4, 2020

Signed-off-by: Julien Pivotto roidelapluie@inuits.eu

@roidelapluie roidelapluie marked this pull request as ready for review June 4, 2020 20:19
@roidelapluie roidelapluie force-pushed the avg_over_time branch 4 times, most recently from 0a140f5 to 095e110 Compare June 4, 2020 20:34
@roidelapluie roidelapluie changed the title Fix avg_over_time for nan Fix avg_over_time for nan and possible float64 overflows Jun 4, 2020
@roidelapluie roidelapluie changed the title Fix avg_over_time for nan and possible float64 overflows Fix avg_over_time for nan and float64 overflows Jun 4, 2020
@roidelapluie roidelapluie force-pushed the avg_over_time branch 2 times, most recently from 51d9eaf to 3035580 Compare June 4, 2020 23:42
@brian-brazil
Copy link
Contributor

brian-brazil commented Jun 5, 2020

Can you explain more what this is trying to do, and if/how it varies from the naieve algorithm?

@roidelapluie
Copy link
Member Author

roidelapluie commented Jun 5, 2020

It fixes avg_over_time for Inf + avg_over_time for float64 that would otherwise overflow the float64.

e.g. the following tests would fail otherwise:

  metric5 1 2 3 Inf Inf
  metric10 -9.988465674311579e+307 9.988465674311579e+307

eval instant at 1m avg_over_time(metric5[1m])
  {} Inf

eval instant at 1m avg_over_time(metric10[1m])
  {} 0

@roidelapluie
Copy link
Member Author

roidelapluie commented Jun 5, 2020

I will make the comments cleaerer

@brian-brazil
Copy link
Contributor

brian-brazil commented Jun 5, 2020

What are those returning now? Does sum_over_time / count_over_time == avg_over_time? Would the avg aggregator need changes too?

@roidelapluie
Copy link
Member Author

roidelapluie commented Jun 5, 2020 •
edited
Loading

Current behaviour

load 10s
  metric5 1 2 3 Inf Inf
  metric6 1 2 3 -Inf -Inf
  metric10 -9.988465674311579e+307 9.988465674311579e+307

eval instant at 1m avg_over_time(metric5[1m])
  {} Nan

eval instant at 1m avg_over_time(metric6[1m])
  {} Nan

eval instant at 1m avg_over_time(metric10[1m])
  {} +Inf

New

load 10s
  metric5 1 2 3 Inf Inf
  metric6 1 2 3 -Inf -Inf
  metric10 -9.988465674311579e+307 9.988465674311579e+307

eval instant at 1m avg_over_time(metric5[1m])
  {} Inf

eval instant at 1m avg_over_time(metric6[1m])
  {} -Inf

eval instant at 1m avg_over_time(metric10[1m])
  {} 0

@brian-brazil
Copy link
Contributor

brian-brazil commented Jun 5, 2020

Why is metric10 changing?

@roidelapluie
Copy link
Member Author

roidelapluie commented Jun 5, 2020

It is one big positive and one big negative float that average to 0

@roidelapluie
Copy link
Member Author

roidelapluie commented Jun 5, 2020

Previously (v.V - mean ) / count would overlow to (Inf) / count => Int

@roidelapluie
Copy link
Member Author

roidelapluie commented Jun 8, 2020

avg_over_time is the same as sum_over_time / count_over_time except for metric9 and metric8 which overflows float64.

@roidelapluie
Copy link
Member Author

roidelapluie commented Jul 8, 2020

@brian-brazil are we interested by this fix?

brian-brazil
brian-brazil reviewed Jul 9, 2020
View reviewed changes
promql/engine.go Outdated
if math.IsInf(group.mean, 0) {
if math.IsInf(s.V, 0) && (group.mean > 0) == (s.V > 0) {
// The `mean` and `s.V` values are `Inf` of the same sign. They
// can't be substracted, but the value of `mean` is correct
Copy link
Contributor

@brian-brazil brian-brazil Jul 9, 2020

Choose a reason for hiding this comment

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

subtracted

promql/engine.go Outdated
break
}
if !math.IsInf(s.V, 0) && !math.IsNaN(s.V) {
// It the mean is an `Inf`, avoid removing an `Inf` with
Copy link
Contributor

@brian-brazil brian-brazil Jul 9, 2020

Choose a reason for hiding this comment

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

I don't understand this comment.

Julien Pivotto added 3 commits July 12, 2020 15:40
Fix avg_over_time with Inf values
b8c2fa0 
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Fix avg and another fix for avg_over_time
7987bf8 
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Address comments
fbf12fc 
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member Author

roidelapluie commented Jul 12, 2020

I have adapted the comment and rebased on top of group().

Remove useless comment
257fd95 
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
brian-brazil
brian-brazil reviewed Jul 13, 2020
View reviewed changes
Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

👍

promql/engine.go Outdated
break
}
if !math.IsInf(s.V, 0) && !math.IsNaN(s.V) {
// At this stage, the mean is an infinite. It the added
Copy link
Contributor

@brian-brazil brian-brazil Jul 13, 2020

Choose a reason for hiding this comment

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

If the

Fix comment
d2d7e80 
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie roidelapluie merged commit d77b56e into prometheus:master Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@brian-brazil brian-brazil brian-brazil left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@roidelapluie @brian-brazil