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

feat: Allow user to handle NaN result in Analysis #977

Merged
merged 13 commits into from
Feb 17, 2021

Conversation

khhirani
Copy link
Contributor

@khhirani khhirani commented Feb 5, 2021

Closes #459

@codecov-io
Copy link

Codecov Report

Merging #977 (84f41d3) into master (671e52d) will decrease coverage by 0.08%.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #977      +/-   ##
==========================================
- Coverage   81.35%   81.26%   -0.09%     
==========================================
  Files         100      100              
  Lines        8837     8859      +22     
==========================================
+ Hits         7189     7199      +10     
- Misses       1180     1186       +6     
- Partials      468      474       +6     
Impacted Files Coverage Δ
metricproviders/wavefront/wavefront.go 83.17% <12.50%> (-4.95%) ⬇️
metricproviders/prometheus/prometheus.go 90.76% <25.00%> (-9.24%) ⬇️
.../apis/rollouts/validation/validation_references.go 74.03% <100.00%> (+0.77%) ⬆️
utils/defaults/defaults.go 100.00% <0.00%> (ø)
utils/replicaset/replicaset.go 89.70% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 671e52d...84f41d3. Read the comment docs.

Signed-off-by: khhirani <kareena.hirani@gmail.com>
Signed-off-by: khhirani <kareena.hirani@gmail.com>
Signed-off-by: khhirani <kareena.hirani@gmail.com>
Signed-off-by: khhirani <kareena.hirani@gmail.com>
Signed-off-by: khhirani <kareena.hirani@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Feb 10, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Conditional approval with improved docs

@@ -643,3 +643,112 @@ spec:
value: "Bearer {{ args.api-token }}"
```

## Handling Metric Results - NaN and Infinity
Metric providers can sometimes return values of NaN (not a number) and infinity. Users can edit the `successCondition` and `failureCondition` fields
Copy link
Member

@jessesuen jessesuen Feb 12, 2021

Choose a reason for hiding this comment

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

I think we should mention the functions in the doc wording. Can you explain the fact that we provide two math functions isNaN and isInf as a convenience to allow users to customize the handling of it and what they return (it may not be obvious to everyone). Also note that if not handled, a NaN result could result in Failed measurements.

Also, please add:

!!! important

    available since v1.0

@khhirani khhirani merged commit b654c88 into argoproj:master Feb 17, 2021
@khhirani khhirani deleted the user-controlled-nan branch February 17, 2021 21:30
@Bunny15738
Copy link

image

Function isNaN(result) giving error.

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.

User controlled handling of NaN metric
4 participants