-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add support for openmetrics 0.0.1 besides 1.0.0 #1640
Conversation
I changed the subject to [WIP] because I intend to add some unit tests of the @fgalan , @AlvaroVega, would you be ok with adding devDependency node-mocks-http? |
Yes, I'm OK with that. |
finally I refactored the code I wanted to test out of the request handler, so I could unit test it without dependencies. The PR i sready for review. |
lib/services/stats/statsRegistry.js
Outdated
for (let part of parts.slice(1)) { | ||
if (part.startsWith('version=')) { | ||
version = part.substring(8).trim(); | ||
} else if (part.startsWith('charset=')) { | ||
charset = part.substring(8).trim(); | ||
} else if (part.startsWith('q=')) { | ||
preference = parseFloat(part.substring(2).trim()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to avoid magic numbers maybe it's better this way:
for (let part of parts.slice(1)) { | |
if (part.startsWith('version=')) { | |
version = part.substring(8).trim(); | |
} else if (part.startsWith('charset=')) { | |
charset = part.substring(8).trim(); | |
} else if (part.startsWith('q=')) { | |
preference = parseFloat(part.substring(2).trim()); | |
let versionTokenLength = 'version='.length; | |
let qTokenLength = 'q='.length; | |
for (let part of parts.slice(1)) { | |
if (part.startsWith('version=')) { | |
version = part.substring(versionTokenLength ).trim(); | |
} else if (part.startsWith('charset=')) { | |
charset = part.substring(versionTokenLength ).trim(); | |
} else if (part.startsWith('q=')) { | |
preference = parseFloat(part.substring(qTokenLength).trim()); |
(Code not actually tested, it may not run, but you get the idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in 679813e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Passing the ball to @AlvaroVega for extra review and LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Continuation to #1627 - The current version of prometheus collector in openshift has been found to request openmetrics 0.0.1