Skip to content

Conversation

@RickVdrongelen
Copy link

⚠️ Please Note: We do not accept all types of pull requests, and we want to ensure we don’t waste your time. Before submitting, make sure you have read our pull request guidelines: Pull Request Rules

❗ Important Announcement

Click here for more details:

🚧 Temporary Delay in Feature Requests and Pull Request Reviews

At this time, we may be slower to respond to new feature requests and review pull requests. Existing requests and PRs will remain in the backlog but may not be prioritized immediately.

  • Reason: Our current focus is on addressing bugs, improving system performance, and implementing essential updates. This will help stabilize the project and ensure smoother management.
  • Impact: While no new feature requests or pull requests are being outright rejected, there may be significant delays in reviews. We encourage the community to help by reviewing PRs or assisting other users in the meantime.
  • What You Can Do: If you're interested in contributing, reviewing open PRs by following our Review Guidelines or offering support to other users is greatly appreciated. All feature requests and PRs will be revisited once the suspension period is lifted.

We appreciate your patience and understanding as we continue to improve Uptime Kuma.

🚫 Please Avoid Unnecessary Pinging of Maintainers

We kindly ask you to refrain from pinging maintainers unless absolutely necessary. Pings are reserved for critical/urgent pull requests that require immediate attention.

Why: Reserving pings for urgent matters ensures maintainers can prioritize critical tasks effectively.

📋 Overview

Provide a clear summary of the purpose and scope of this pull request:

  • What problem does this pull request address?

Currently it is not possible to use the tags assigned to a monitor as a prometheus metric. This can be a real problem for users that would like to use these tags as a method to filter upstream. For example, for advanced notifications for an project name/key. Or allowing to notify if something in the server infrastructure went down, instead of notifying for every single monitor using that specific tag.

  • What features or functionality does this pull request introduce or enhance?

This pull request adds the ability to add multiple tags and corresponding values per monitor metric. Before these tags are added, they are first checked if they use non-ascii characters, and if they do, the tag is not added to the monitor. (See #4704 (comment))

🔄 Changes

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify):
    • Provide additional details here.

🔗 Related Issues

📄 Checklist *

  • 🔍 My code adheres to the style guidelines of this project.
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

ℹ️ Additional Context

Provide any relevant details to assist reviewers in understanding the changes.

Click here for more details:

Key Considerations:

  • Design decisions – Key choices or trade-offs made during development.
  • Alternative solutions – Approaches considered but not implemented, along with reasons.
  • Relevant links – Specifications, discussions, or resources that provide context.
  • Dependencies – Related pull requests or issues that must be resolved before merging.
  • Additional context – Any other details that may help reviewers understand the changes.

Provide details here

CommanderStorm

This comment was marked as resolved.

Comment on lines 69 to 76
if (asciiRegex.test(tag.name)) {
if (tag.value !== "" && asciiRegex.test(tag.value)) {
filteredTags.push(`${tag.name}:${tag.value}`);
} else {
filteredTags.push(tag.name);
}
}
return filteredTags;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of filtering, please just drop the non-matching characters after building the tag.

Current algorithm also does not work. ${tag.name}:${tag.value} is not a valid tag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To skip the arguing here, please add a testcase to make sure that this method works as intended.

Copy link
Collaborator

Choose a reason for hiding this comment

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

below is the sanatisation logic that I would expect

let tagText = tag.value ? `${tag.name}:${tag.value}` : tag.name;
tagText = tagText.replace(/[^a-zA-Z0-9_]/g, "")
tagText = tagText.replace(/^[^a-zA-Z_]+/, "")
if (tagText) {
    resultingTags.push(tagText)
}

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Thanks for looking at my code and providing feedback. However, i have some concerns.

Personally I think, it is really weird and unexpected behaviour if a tag with non-ascii characters is suddenly shortened without the ascii characters. It makes more sense to me, to ignore those tags and not include them into the metrics, then to include tags which can possibly end up making no sense at all.

Also, in the last code block you provided a multiple regex replaces, which would also remove the colon (:) i provided as a seperator for the value. You also said the ${tag.name}:${tag.value} is incorrect before. I am okay with removing the tag-value from the metrics. Right now I have no use-case for it, but i can imagine someone else might have. And since a colon is valid ascii, I see no reason why prometheus would not be able to parse the tag-value metric.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is valid ascii is not relavant.

https://prometheus.io/docs/concepts/data_model/

Something that we did not notice: Since prometheus v3, UTF8 may be supported.

I have no clue if this means that our metrics library does still enforce [a-zA-Z_][a-zA-Z0-9_]* as the supported regex-invariant that needs to be upheld after this function for each sanatised tag.
It definitvely did enforce this in october:

Our metrics library dying causes all monitors to stop not working.

Will need to test if filtering is nessesary after bumping the metrics dependency
You can do this testing if you get to it first.

In any case, we either support [a-zA-Z_][a-zA-Z0-9_]* or UTF8. Nothing in between like you proposed, that just causes issues.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, thanks for checking. I will update my PR according to the given regex, and that probably means that the value functionality needs to go.

@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label May 27, 2025
CommanderStorm

This comment was marked as resolved.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

tested and this works.

Thanks for the contribution
image

@CommanderStorm CommanderStorm marked this pull request as ready for review June 24, 2025 15:32
@CommanderStorm CommanderStorm added this to the 2.0.0-beta.4 milestone Jun 24, 2025
@CommanderStorm
Copy link
Collaborator

actually.. wait a minute..
Is this scheme what others want?

monitor_tags="foo_foo,bar_bar,bar_baz"

Would the following not be better?

foo="foo",bar="bar,baz"

CC people who have expressed interest in this:
@mabed-fr @ldubrois @spali @xlr-8

@CommanderStorm CommanderStorm added question Further information is requested pr:depends on other pending other things to be done first and removed pr:please address review comments this PR needs a bit more work to be mergable labels Jun 24, 2025
@mabed-fr
Copy link

Hello,
Thanks for the work on this!

Regarding the choice of format: the second option — using separate labels like foo="foo", bar="bar,baz" — is much more appropriate for Prometheus and especially for usage with Grafana.

Prometheus is designed to work with individual key-value labels. This allows for proper filtering, grouping (e.g., sum by(...)), and querying without needing to parse strings manually. Grafana also relies heavily on this label structure to provide flexible dashboards and filtering capabilities.

Using a single label like monitor_tags="foo_foo,bar_bar" would make filtering and aggregating significantly more difficult, and goes against Prometheus best practices.

So I’d strongly recommend going with the second approach.

@CommanderStorm CommanderStorm added pr:please address review comments this PR needs a bit more work to be mergable and removed question Further information is requested pr:depends on other pending other things to be done first labels Jun 25, 2025
@CommanderStorm CommanderStorm removed this from the 2.0.0-beta.4 milestone Jun 25, 2025
@RickVdrongelen
Copy link
Author

Thanks for your response! I will update it to the second approach.

@mabed-fr
Copy link

Thank you very much for your feedback and update!
I'm patiently waiting for this feature.

@ldubrois
Copy link

Hi

I confirm the second option, with several tags , is way better for prometheus.

Thanks

@RickVdrongelen
Copy link
Author

Soooo, i did a thing.

The prometheus metrics now supports monitor tags as labels with the possible value(s) as label values. However, it wasn't as straightforward as i had hoped.
The original prometheus.js didn't support adding commonLabels to the metrics, but that was needed to add all the possible tags. I added a initialise static method to the Prometheus class, which is called when starting the application. This method gets all possible tags, and adds them to the commonLabels, and after that all the metrics get registered.

As long as the tag exists and is added to the monitor, it is shown in the metrics. With none, or all values being comma seperated. I tested what happens when adding tags, and i am glad to report that when a existing tag is added to a monitor, the metrics get updated after just a refresh. Sadly this is not supported when adding tags that did not exist when starting the application. Then the tag is just not added, and a restart is required. Please let me know if you have some insight if you know how to possibly fix this.

And also, please let me know if you have some feedback on the extra added code. (server-side) JS is not my strong-point, so any help is gladly accepted.

CommanderStorm

This comment was marked as resolved.

…event overwriting very much needed metric labels.
@RickVdrongelen
Copy link
Author

Thanks for the review and feedback! Ordering is now done alphabetically to both the tags and tag values, and they no longer are able to potentially overwrite the already existing labels.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Sorry to give you another round, but after reviewing the code I found that null values would not work

Should be simple enough to fix though

Tip

For the others in this thread:
you can test this change via running

npx kuma-pr RickVdrongelen:feature/add-tags-to-metrics

@CommanderStorm CommanderStorm changed the title feat #618: add tags to prometheus metrics feat(prometheus): add tags to prometheus metrics Aug 4, 2025
@CommanderStorm CommanderStorm added pr:needs review this PR needs a review by maintainers or other community members and removed pr:please address review comments this PR needs a bit more work to be mergable labels Aug 4, 2025
@leonboot
Copy link

I would very much like to see this feature appear in a near-future release! 👍

@CommanderStorm
Copy link
Collaborator

Feel free to test this PR to look if it is bug-free.
That is what this PR is blocked on: testing if it has another bug and works as Prometheus users expect.
I don't use Prometheus, so cannot really review if it is what users expect.

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

Labels

pr:needs review this PR needs a review by maintainers or other community members

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend existing Prometheus labels to include tags

5 participants