Skip to content

feat(prometheus): Include tags in metrics labels#7812

Closed
carnei-ro wants to merge 1 commit intoKong:masterfrom
carnei-ro:feat/prometheus/include-tags-in-metrics-labels
Closed

feat(prometheus): Include tags in metrics labels#7812
carnei-ro wants to merge 1 commit intoKong:masterfrom
carnei-ro:feat/prometheus/include-tags-in-metrics-labels

Conversation

@carnei-ro
Copy link
Copy Markdown
Contributor

@carnei-ro carnei-ro commented Sep 6, 2021

Moving Kong/kong-plugin-prometheus#149 to here

closes #7678

@carnei-ro carnei-ro force-pushed the feat/prometheus/include-tags-in-metrics-labels branch 2 times, most recently from 6e6ab6c to 2362fb3 Compare September 6, 2021 17:52
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if the sorting order of tags in Kong's DAO is stable or not i.e. can the order of tags change or not.
If it can change, I'd recommend sorting them because otherwise, it will lead to unnecessary duplication of metrics in the Prometheus store.

This is also additional table pollution probably. Wangchong will know best how to tackle that part.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right! I added sorting 😄

@carnei-ro carnei-ro force-pushed the feat/prometheus/include-tags-in-metrics-labels branch from 2362fb3 to 64958e8 Compare September 9, 2021 12:21
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a test case for when there are tags on the service or route. Please make sure to have multiple tags to test for sort order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wrote all those tests at 02-access_spec.lua, there is a scenario for: exposing all tags, only services and only route. Do you think I should replicate it in here?

@carnei-ro carnei-ro force-pushed the feat/prometheus/include-tags-in-metrics-labels branch 2 times, most recently from 371c387 to 794c37e Compare October 12, 2021 21:11
@rmrf
Copy link
Copy Markdown

rmrf commented Dec 22, 2021

how this going

@mayocream
Copy link
Copy Markdown
Contributor

@carnei-ro hi, any updates?

@carnei-ro
Copy link
Copy Markdown
Contributor Author

hey guys, I'm not really sure why some tests are failing - need help

@carnei-ro carnei-ro force-pushed the feat/prometheus/include-tags-in-metrics-labels branch from 794c37e to dd870f8 Compare January 24, 2022 20:09
@mayocream
Copy link
Copy Markdown
Contributor

mayocream commented Feb 21, 2022

Hi @carnei-ro

IMO, I don't think this behavior should be included in promethues plugin officially.

  1. We might want to avoid adding empty labels like service_tags="",route_tags="" by default
  2. Don't overuse labels, cardinality can be very large

ref:

@guanlan guanlan requested a review from a team as a August 25, 2022 01:38
@hbagdi
Copy link
Copy Markdown
Contributor

hbagdi commented Oct 25, 2022

We won't be able to accept this contribution at this point. The cardinality issues in the plugin have been plaguing Kong's performance for quite some time. Please revisit this change in 6 months.

@hbagdi hbagdi closed this Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include tags in metrics labeles

6 participants