You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/11/09 09:28:44 UTC
[GitHub] [apisix] bisakhmondal opened a new pull request #5463: feat: introducing prefer_name attribute in datadog plugin
bisakhmondal opened a new pull request #5463:
URL: https://github.com/apache/apisix/pull/5463
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
As discussed offline with @spacewander, this PR adds an additional attribute in plugin schema `prefer_name` to provide the flexibility of choosing route/service id over the name (in case of name conflict/user preference).
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #5463:
URL: https://github.com/apache/apisix/pull/5463
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #5463:
URL: https://github.com/apache/apisix/pull/5463#discussion_r745536647
##########
File path: t/plugin/datadog.t
##########
@@ -395,3 +395,141 @@ message received: apisix\.apisix\.latency:[\d.]+\|h\|#source:apisix,new_tag:must
message received: apisix\.ingress\.size:[\d]+\|ms\|#source:apisix,new_tag:must,route_name:1,balancer_ip:[\d.]+,response_status:200,scheme:http
message received: apisix\.egress\.size:[\d]+\|ms\|#source:apisix,new_tag:must,route_name:1,balancer_ip:[\d.]+,response_status:200,scheme:http
/
+
+
+
+=== TEST 8: testing behaviour with service id
Review comment:
Yes. Both enabled: test 2- 6
Prefer_name(true), route_name(false) - test 7
prefer_name(false), route_name(true) - Test-9 (route is created at test-8 with route_name)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5463:
URL: https://github.com/apache/apisix/pull/5463#discussion_r745477999
##########
File path: t/plugin/datadog.t
##########
@@ -395,3 +395,141 @@ message received: apisix\.apisix\.latency:[\d.]+\|h\|#source:apisix,new_tag:must
message received: apisix\.ingress\.size:[\d]+\|ms\|#source:apisix,new_tag:must,route_name:1,balancer_ip:[\d.]+,response_status:200,scheme:http
message received: apisix\.egress\.size:[\d]+\|ms\|#source:apisix,new_tag:must,route_name:1,balancer_ip:[\d.]+,response_status:200,scheme:http
/
+
+
+
+=== TEST 8: testing behaviour with service id
Review comment:
Is there a test for prefer_name + the route name?
##########
File path: apisix/plugins/datadog.lua
##########
@@ -83,15 +85,24 @@ local function generate_tag(entry, const_tags)
tags = {}
end
- -- priority on route name, if not found using the route id.
- if entry.route_name ~= "" then
+ -- priority on route_name & prefer_name, if not found fall back to the route id
+ if entry.prefer_name and entry.route_name ~= "" then
core.table.insert(tags, "route_name:" .. entry.route_name)
elseif entry.route_id and entry.route_id ~= "" then
core.table.insert(tags, "route_name:" .. entry.route_id)
end
if entry.service_id and entry.service_id ~= "" then
- core.table.insert(tags, "service_id:" .. entry.service_id)
+ local svc_id = entry.service_id
+
+ if entry.prefer_name then
+ local svc = service_fetch(svc_id)
Review comment:
To prevent race and make the report code straightforward, it would be better to fetch the name in the log phase instead of the report process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5463:
URL: https://github.com/apache/apisix/pull/5463#discussion_r746204195
##########
File path: apisix/plugins/datadog.lua
##########
@@ -144,6 +149,12 @@ function _M.log(conf, ctx)
entry.balancer_ip = ctx.balancer_ip or ""
entry.route_name = ctx.route_name or ""
entry.scheme = ctx.upstream_scheme or ""
+ entry.prefer_name = conf.prefer_name
+ -- capture service name at request response cycle if prefer_name is set.
+ if entry.prefer_name and entry.service_id and entry.service_id ~= "" then
+ local svc = service_fetch(entry.service_id)
+ entry.service_name = svc and svc.value.name or ""
Review comment:
```suggestion
entry.service_id = svc and svc.value.name or ""
```
So there is no need to check the service_name in the report process.
And we can do the same thing with the route, so there is no need to pass the prefer_name configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5463:
URL: https://github.com/apache/apisix/pull/5463#discussion_r746292328
##########
File path: apisix/plugins/datadog.lua
##########
@@ -144,17 +141,22 @@ function _M.log(conf, ctx)
local entry = fetch_log(ngx, {})
entry.upstream_latency = ctx.var.upstream_response_time * 1000
entry.balancer_ip = ctx.balancer_ip or ""
- entry.route_name = ctx.route_name or ""
entry.scheme = ctx.upstream_scheme or ""
entry.prefer_name = conf.prefer_name
- -- capture service name at request response cycle if prefer_name is set.
- if entry.prefer_name and entry.service_id and entry.service_id ~= "" then
- local svc = service_fetch(entry.service_id)
+ -- if prefer_name is set, fetch the service/route name. If the name is nil, fall back to id.
+ if entry.prefer_name then
Review comment:
We can use `conf.prefer_name` directly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #5463:
URL: https://github.com/apache/apisix/pull/5463#discussion_r746241234
##########
File path: apisix/plugins/datadog.lua
##########
@@ -144,6 +149,12 @@ function _M.log(conf, ctx)
entry.balancer_ip = ctx.balancer_ip or ""
entry.route_name = ctx.route_name or ""
entry.scheme = ctx.upstream_scheme or ""
+ entry.prefer_name = conf.prefer_name
+ -- capture service name at request response cycle if prefer_name is set.
+ if entry.prefer_name and entry.service_id and entry.service_id ~= "" then
+ local svc = service_fetch(entry.service_id)
+ entry.service_name = svc and svc.value.name or ""
Review comment:
Ack. Done. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5463: feat: introducing prefer_name attribute in datadog plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #5463:
URL: https://github.com/apache/apisix/pull/5463#discussion_r745534766
##########
File path: apisix/plugins/datadog.lua
##########
@@ -83,15 +85,24 @@ local function generate_tag(entry, const_tags)
tags = {}
end
- -- priority on route name, if not found using the route id.
- if entry.route_name ~= "" then
+ -- priority on route_name & prefer_name, if not found fall back to the route id
+ if entry.prefer_name and entry.route_name ~= "" then
core.table.insert(tags, "route_name:" .. entry.route_name)
elseif entry.route_id and entry.route_id ~= "" then
core.table.insert(tags, "route_name:" .. entry.route_id)
end
if entry.service_id and entry.service_id ~= "" then
- core.table.insert(tags, "service_id:" .. entry.service_id)
+ local svc_id = entry.service_id
+
+ if entry.prefer_name then
+ local svc = service_fetch(svc_id)
Review comment:
Thanks for pointing it out. Since everything is hot loaded it could be a mess. Updating.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org