feat(prometheus): Include tags in metrics labels #7812
Conversation
carnei-ro
force-pushed
the
feat/prometheus/include-tags-in-metrics-labels
branch
2 times, most recently
from
6e6ab6c
to
2362fb3
Compare
kong/plugins/prometheus/exporter.lua
Outdated
| if message and message.route then | ||
| route_name = message.route.name or message.route.id | ||
| route_tags = ((message.route.tags) and (type(message.route.tags) == "table")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right! I added sorting 😄
carnei-ro
force-pushed
the
feat/prometheus/include-tags-in-metrics-labels
branch
from
2362fb3
to
64958e8
Compare
| @@ -232,7 +232,7 @@ describe("Plugin: prometheus (access via status API)", function() | |||
| path = "/metrics", | |||
| }) | |||
| local body = assert.res_status(200, res) | |||
| return body:find('kong_http_status{service="mock-grpcs-service",route="grpcs-route",code="200"} 1', nil, true) | |||
| return body:find('kong_http_status{service="mock-grpcs-service",route="grpcs-route",code="200",service_tags="",route_tags=""} 1', nil, true) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
feat/prometheus/include-tags-in-metrics-labels
branch
2 times, most recently
from
371c387
to
794c37e
Compare
|
how this going |
|
@carnei-ro hi, any updates? |
|
hey guys, I'm not really sure why some tests are failing - need help |
carnei-ro
force-pushed
the
feat/prometheus/include-tags-in-metrics-labels
branch
from
794c37e
to
dd870f8
Compare
|
Hi @carnei-ro IMO, I don't think this behavior should be included in promethues plugin officially.
ref: |
|
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. |
Moving Kong/kong-plugin-prometheus#149 to here
closes #7678