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 2022/08/30 13:44:50 UTC
[GitHub] [apisix] jaysonsantos opened a new pull request, #7822: feat: Add ability to inject headers to otel traces
jaysonsantos opened a new pull request, #7822:
URL: https://github.com/apache/apisix/pull/7822
### Description
This adds a new feature that allows to inject headers into traces, like, if you have internal headers that would make sense to live as an attribute like x-client-uuid, x-client-version.
It also adds the ability to using prefixes to get the variables, like: `x-client-*`
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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 diff in pull request #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r962304595
##########
Makefile:
##########
@@ -460,3 +468,17 @@ ci-env-down:
@$(call func_echo_status, "$@ -> [ Start ]")
$(ENV_DOCKER_COMPOSE) down
@$(call func_echo_success_status, "$@ -> [ Done ]")
+
+
+### eclint: Validate files against editorconfig
Review Comment:
Could you move the dev kit stuff to a separate PR? Thanks!
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -169,6 +169,13 @@ local schema = {
type = "string",
minLength = 1,
}
+ },
+ additional_header_attributes = {
Review Comment:
Better to rename this conf to show it is used for prefix. Otherwise, people will ask why `additional_attributes` can use `http_header` but we have another conf called additional_header_attributes.
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -273,6 +280,27 @@ local function create_tracer_obj(conf)
end
+local function inject_attributes(attributes, wanted_attributes, source, with_prefix)
+ for _, key in ipairs(wanted_attributes) do
+ local is_key_a_match = #key >= 2 and key:sub(-1, -1) == "*" and with_prefix
Review Comment:
`key:sub(-1, -1)`
Would be better to use `:byte`?
--
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] tzssangglass commented on a diff in pull request #7822: feat: Add ability to inject headers to otel traces
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r959447194
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -273,6 +280,25 @@ local function create_tracer_obj(conf)
end
+local function inject_attributes(attributes, wanted_attributes, source)
+ for _, key in ipairs(wanted_attributes) do
+ local is_key_a_match = #key >= 2 and string.sub(key, -1, -1) == "*"
Review Comment:
```suggestion
local is_key_a_match = #key >= 2 and key:sub(-1, -1) == "*"
```
--
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] jaysonsantos commented on a diff in pull request #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
jaysonsantos commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r962607031
##########
Makefile:
##########
@@ -460,3 +468,17 @@ ci-env-down:
@$(call func_echo_status, "$@ -> [ Start ]")
$(ENV_DOCKER_COMPOSE) down
@$(call func_echo_success_status, "$@ -> [ Done ]")
+
+
+### eclint: Validate files against editorconfig
Review Comment:
i've dropped the commit
--
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 pull request #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7822:
URL: https://github.com/apache/apisix/pull/7822#issuecomment-1235100710
> @spacewander do you prefer me to change the function `inject_attributes` to have a parameter like `with_prefix` and when calling with ctx.var, i could send it as false but true for headers?
LGTM
--
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] tokers commented on a diff in pull request #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r961484339
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -273,6 +280,25 @@ local function create_tracer_obj(conf)
end
+local function inject_attributes(attributes, wanted_attributes, source)
+ for _, key in ipairs(wanted_attributes) do
+ local is_key_a_match = #key >= 2 and key:sub(-1, -1) == "*"
+ local prefix = key:sub(0, -2)
Review Comment:
We can only calculate the `prefix` only if `is_key_a_match` is `true` since it will generate a new string object. What about re-organizing the code to:
```lua
local is_prefix_key = #key >= 2 and key:byte(-1, -1) == str_byte("*")
if is_prefix_key then
local prefix = key:sub(0, -2)
for possible_key, value in pairs(source) do
if core.string.has_prefix(possible_key, prefix) then
core.table.insert(attributes, attr.string(possible_key, value))
end
end
else
local val = source[key]
if val then
core.table.insert(attributes, attr.string(key, val))
end
end
```
Note it's just a pseudo code.
--
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] starsz commented on a diff in pull request #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r961476691
##########
docs/en/latest/plugins/opentelemetry.md:
##########
@@ -46,6 +46,8 @@ The Plugin only supports binary-encoded [OLTP over HTTP](https://opentelemetry.i
| sampler.options.root.options.fraction | number | False | 0 | [0, 1] | Root sampling probability for `trace_id_ratio`. |
| additional_attributes | array[string] | False | | | Variables and its values which will be appended to the trace span. |
| additional_attributes[0] | string | True | | | APISIX or Nginx variables. For example, `http_header` or `route_id`. |
+| additional_header_attributes | array[string] | False | | | Variables and its values which will be appended to the trace span. |
+| additional_header_attributes[0] | string | True | | | Request headers. For example, `x-my-header"` or `x-my-headers-*` to include all headers with the prefix `x-my-headers-`. |
Review Comment:
```suggestion
| additional_header_attributes[0] | string | True | | | Request headers. For example, `x-my-header` or `x-my-headers-*` to include all headers with the prefix `x-my-headers-`. |
```
Haha.
--
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] jaysonsantos commented on pull request #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
jaysonsantos commented on PR #7822:
URL: https://github.com/apache/apisix/pull/7822#issuecomment-1234257448
@spacewander do you prefer me to change the function `inject_attributes` to have a parameter like `with_prefix` and when calling with ctx.var, i could send it as false but true for headers?
--
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 #7822: feat: Add ability to inject headers via prefix to otel traces
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7822:
URL: https://github.com/apache/apisix/pull/7822
--
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] tokers commented on a diff in pull request #7822: feat: Add ability to inject headers to otel traces
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r959066037
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -273,6 +280,26 @@ local function create_tracer_obj(conf)
end
+local function inject_attributes(attributes, wanted_attributes, source)
+ for _, key in ipairs(wanted_attributes) do
+ local is_key_a_match = #key >= 2 and string.sub(key, -1, -1) == "*"
+ local prefix = string.sub(key, 0, -2)
+ local prefix_size = #prefix
+ local val = source[key]
+ if val then
+ core.table.insert(attributes, attr.string(key, val))
+ end
+ if is_key_a_match then
+ for possible_key, value in pairs(source) do
+ if string.sub(possible_key, 0, prefix_size) == prefix then
Review Comment:
We already have a `has_prefix` method in `apisix.core.string`.
--
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] tzssangglass commented on a diff in pull request #7822: feat: Add ability to inject headers to otel traces
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7822:
URL: https://github.com/apache/apisix/pull/7822#discussion_r959447521
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -273,6 +280,25 @@ local function create_tracer_obj(conf)
end
+local function inject_attributes(attributes, wanted_attributes, source)
+ for _, key in ipairs(wanted_attributes) do
+ local is_key_a_match = #key >= 2 and string.sub(key, -1, -1) == "*"
+ local prefix = string.sub(key, 0, -2)
Review Comment:
```suggestion
local prefix = key:sub(0, -2)
```
--
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