feat(plugins)(datadog): add a datadog tag for the route name to each metric #13494
Conversation
github-actions
bot
added
plugins/datadog
schema-change-noteworthy
cherry-pick kong-ee
tysoekong
force-pushed
the
feat/datadog-plugin-route-tag
branch
2 times, most recently
from
6a5aca7
to
1bfc807
Compare
kong/plugins/datadog/handler.lua
Outdated
| local result = { | ||
| (conf.service_name_tag or "name") .. ":" .. service_name, | ||
| (conf.status_tag or "status") .. ":" .. status | ||
| (conf.status_tag or "status") .. ":" .. status, | ||
| route_name and route_name ~= "" and ((conf.route_name_tag or "route") .. ":" .. route_name) or nil |
There was a problem hiding this comment.
is it desidable that route_name is nil when not available or should it have some default? I'm thinking of what would happen if/when we add another value to this result array, then having a nil element in the middle of the array could affect iterations, although it seems we're passing this to generate a statsd_message, not sure how it would be handled there.
I'd like it better if we could keep this consistent with the other metrics and just do: (conf.route_name_tag or "route") .. ":" .. route_name here while passing some default value to this function to ensure route_name is not nil, wdyt?
There was a problem hiding this comment.
Not sure what we can set for default, it might be confusing. But yeah you're right if null/missing, it might bust our user's graphs, for example.
Maybe the route ID instead?
There was a problem hiding this comment.
yeah that would somehow be similar to what we do with the service name (falls back to host), I think it's a good idea.
There was a problem hiding this comment.
@samugi Okay the logic is now:
- route must actually be present - nil check
- if route.name is there: gsub the dots away for _
- if route.name is not there, use route.id
- otherwise route is nil for some reason, fall back to empty string
That should do it?
team-eng-enablement
added
author/community
|
@tysoekong @samugi is this going to be included in 3.8? |
it shouldn't be @GGabriele , the feature freeze ship has sailed, so I think it's correct that the milestone is empty on this one |
team-gateway-bot
added
the
author/community
tysoekong
force-pushed
the
feat/datadog-plugin-route-tag
branch
from
1bfc807
to
74333fc
Compare
tysoekong
force-pushed
the
feat/datadog-plugin-route-tag
branch
from
74333fc
to
9afb491
Compare
| consumer_id, metric_config.tags, conf) | ||
| consumer_id, metric_config.tags, | ||
| conf, | ||
| message.route and (message.route.name and gsub(message.route.name ~= null and |
There was a problem hiding this comment.
is this passing false to gsub when message.route.name is null?
When it gets this complex I'd rather be verbose and write down the logic above, assigning the final value to some local variable, and then just pass it to compose_tags, wdyt?
| local result = { | ||
| (conf.service_name_tag or "name") .. ":" .. service_name, | ||
| (conf.status_tag or "status") .. ":" .. status | ||
| (conf.status_tag or "status") .. ":" .. status, | ||
| route_name and route_name ~= "" and ((conf.route_name_tag or "route") .. ":" .. route_name) |
There was a problem hiding this comment.
I think the checks route_name and route_name ~= "" can be omitted right? We know it'll either have a value or be "", and in case it's "" we probably want to just use it like that, or else this evaluation will return false, am I right?
Summary
This adds a "route name" tag to each of the Datadog metric outputs, just as it does with service, consumer, status code, and more.
One of our favourite users has been asking for this for ages, so we wanted to help by contribute this back instead of creating them a custom OSS build.
Checklist
changelog/unreleased/kongorskip-changeloglabel added on PR if changelog is unnecessary. README.md