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

Recent metrics filter change broke metrics collection for non-scientific values #1754

Closed
RDarrylR opened this issue Dec 1, 2021 · 2 comments · Fixed by #1755
Closed

Recent metrics filter change broke metrics collection for non-scientific values #1754

RDarrylR opened this issue Dec 1, 2021 · 2 comments · Fixed by #1755

Comments

@RDarrylR
Copy link

RDarrylR commented Dec 1, 2021

/kind bug

What steps did you take and what happened:
The DefaultFilter value for the File Metrics Collector was recently changed to fix issues parsing scientific notation values but with the change multi digit number values are no longer parsed correctly and are truncated at the first digit.

What did you expect to happen:
Parsing of normal number values would work correctly.

Anything else you would like to add:

My metrics values that used to parse correctly are now truncated at the first digit. You can see it using a tool like https://regex101.com/

Old filter = ([\w|-]+)\s*=\s*((-?\d+)(\.\d+)?)

New filter = ([\w|-]+)\s*=\s*([+-]?\d(\.\d+)?([Ee][+-]?\d+)?)

Metrics like the following don't get parsed correctly with the new filter - they only match the first digit

metric1=888
metric2=888.33

metrics collector will get just 8 for these metrics with the new filter

Environment:

  • Katib version (check the Katib controller image version): latest from master
  • Kubernetes version: (kubectl version): 1.18.20
  • OS (uname -a): ubuntu 20.04

Impacted by this bug? Give it a 👍 We prioritize the issues with the most 👍

@andreyvelich
Copy link
Member

Thank you so much for finding this @RDarrylR!
I think we forget to add + in the regex.

I would suggest that we use this regex to support -.111 format also.

([\w|-]+)\s*=\s*([+-]?\d*(\.\d+)?([Ee][+-]?\d+)?)

WDYT @RDarrylR @kubeflow/wg-automl-leads @anencore94 ?

/assign @andreyvelich
/priority p0

@RDarrylR
Copy link
Author

RDarrylR commented Dec 1, 2021

@andreyvelich That new version seems to work for all the cases I'm using.

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

Successfully merging a pull request may close this issue.

2 participants