Skip to content

Conversation

@sondrelg
Copy link

@sondrelg sondrelg commented Mar 24, 2025

Handle NaN values

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

Hi!

I have this analysis metric:

- name: ingress success rate
  successCondition: len(result) == 0 || isNaN(result[0]) || result[0] >= 0.99  # allow some room for rounding error
  failureLimit: 3
  inconclusiveLimit: 10
  consecutiveSuccessLimit: 5
  interval: 1m
  provider:
    prometheus:
      address: "{{args.prometheus-address}}"
      query: >-
        sum(rate(nginx_ingress_controller_requests{ingress="service-ingress", status!~"[5].*|499"}[1m]))
        /
        sum(rate(nginx_ingress_controller_requests{ingress="service-ingress", status_code!~"499"}[1m]))

Which returns data like this:

{
  "value": "[NaN]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:25:22Z"
},
{
  "value": "[NaN]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:26:22Z"
},
{
  "value": "[1]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:27:22Z"
},
{
  "value": "[1]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:28:22Z"
}

When I try to view the analysis in the dashboard, the rendering fails and I see this error in the console:

image

I believe this fixes the issue, though I haven't written the tests to prove it yet - sorry about that!

I'm on version 1.8.1.

Handle NaN values

Signed-off-by: Sondre Lillebø Gundersen <sondrelg@live.no>
@sonarqubecloud
Copy link

@sondrelg
Copy link
Author

I see now that this was fixed in the past: #3801. Seems to have been rolled back at some point?

@github-actions
Copy link
Contributor

Published E2E Test Results

  4 files    4 suites   3h 16m 56s ⏱️
115 tests 108 ✅  7 💤 0 ❌
460 runs  432 ✅ 28 💤 0 ❌

Results for commit a5a9be5.

@github-actions
Copy link
Contributor

Published Unit Test Results

2 307 tests   2 307 ✅  2m 59s ⏱️
  129 suites      0 💤
    1 files        0 ❌

Results for commit a5a9be5.

@sondrelg
Copy link
Author

@ashutosh16, sorry for the ping, but I see you removed the json5.parse call in #3881. Was that intentional?

Not sure, but #3801 seems like it addressed the problem I'm having, so while I'm not sure why, I think the reversal might have reintroduced the issue

@ashutosh16
Copy link
Contributor

ashutosh16 commented Apr 9, 2025

@sondrelg Dashboard was crashing even after #3801 , I don't exactly remember what JSON5 parse was returning, so i tried to fix it by adding try/block which worked. Verified below data is parse correctly in the dashboard, Are you trying to parse different data?

        - finishedAt: '2025-04-08T14:47:53Z'
          phase: Failed
          startedAt: '2025-04-08T14:47:53Z'
          value: '[NaN]'

@sondrelg
Copy link
Author

sondrelg commented Apr 9, 2025

The problematic data seems to look like this, for me:

{
  "value": "[NaN]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:25:22Z"
},
{
  "value": "[NaN]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:26:22Z"
},
{
  "value": "[1]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:27:22Z"
},
{
  "value": "[1]",
  "status": "Successful",
  "metricName": "ingress success rate",
  "startedAt": "2025-03-24T13:28:22Z"
}

Seems pretty similar

@ashutosh16
Copy link
Contributor

@sondrelg Not sure why it is failing for you, your changes look reliable to me. Were you able to verify the change in your setup?

let parsedValue;
try {
parsedValue = JSON.parse(value);
parsedValue = JSON.parse(safeValue);
Copy link
Contributor

@ashutosh16 ashutosh16 Apr 11, 2025

Choose a reason for hiding this comment

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

If the value is null then we can skip the parse and safely ignore the value or set it to 0

@sondrelg
Copy link
Author

sondrelg commented Jul 1, 2025

Yes the changes fixed the problem on my setup, as far as I can tell 👍

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.

2 participants