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

Add prometheus prefix solution #1001

Merged
merged 5 commits into from
May 4, 2023
Merged

Add prometheus prefix solution #1001

merged 5 commits into from
May 4, 2023

Conversation

wirednkod
Copy link
Contributor

Fixes #974

@wirednkod wirednkod requested review from pepoviola and l0r1s and removed request for pepoviola May 3, 2023 12:14
Copy link
Contributor

@l0r1s l0r1s left a comment

Choose a reason for hiding this comment

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

Some contestable remarks but lgtm 👍

Comment on lines 214 to 219
if (nodeGroup.prometheus_prefix) {
node.prometheus_prefix = nodeGroup.prometheus_prefix;
} else if (config.relaychain.prometheus_prefix) {
node.prometheus_prefix = config.relaychain.prometheus_prefix;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this ?

Suggested change
if (nodeGroup.prometheus_prefix) {
node.prometheus_prefix = nodeGroup.prometheus_prefix;
} else if (config.relaychain.prometheus_prefix) {
node.prometheus_prefix = config.relaychain.prometheus_prefix;
}
node.prometheus_prefix = nodeGroup.prometheus_prefix || config.relaychain.prometheus_prefix;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really; cause there is a chance that none of them exist, and in that case I want to avoid adding the key "prometheus_prefix" to the object

Comment on lines 634 to 635
if (node.prometheus_prefix)
nodeSetup.prometheusPrefix = node.prometheus_prefix;
Copy link
Contributor

Choose a reason for hiding this comment

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

If prometheus_prefix is some value || undefined it will stay undefined

Suggested change
if (node.prometheus_prefix)
nodeSetup.prometheusPrefix = node.prometheus_prefix;
nodeSetup.prometheusPrefix = node.prometheus_prefix;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really - prior to this if the key prometheusPrefix does not exist; With your suggestion, it will create the key and "assign" undefined to it

Comment on lines 130 to 131
export function getProcessStartTimeKey(prefix = "substrate") {
return `${prefix}_process_start_time_seconds`;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@pepoviola
Copy link
Collaborator

thanks @wirednkod, looks good! I think we can take another approach by set the prefix (with substrate as default) if not set by config, this will save us some if statements down the road :)
If you are agree, I can make the modifications.

Thanks!

@kamilsa kamilsa mentioned this pull request May 4, 2023
3 tasks
@pepoviola pepoviola merged commit 84763c7 into main May 4, 2023
@pepoviola pepoviola deleted the nik-fix-prometheusprefix branch May 4, 2023 21:48
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.

Customizable metrics prefixes
3 participants