feat: Add ability to inject headers via prefix to otel traces #7822

jaysonsantos · 2022-08-30T13:44:46Z

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

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

tokers · 2022-08-31T01:43:19Z

apisix/plugins/opentelemetry.lua

+        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


We already have a has_prefix method in apisix.core.string.

apisix/plugins/opentelemetry.lua

Co-authored-by: tzssangglass <tzssangglass@gmail.com>

spacewander

I am afraid we can't iterate the ctx.var and filter with prefixes to get the variables as it uses a lazy-load mechanism. The wanted attribute may not be loaded when we iterate this object.

Moreover, we already support injecting header via variable http_x_client_version.

However, the headers can be filtered by prefixes. So there is still a new feature "inject headers via prefix". Maybe you can change the title of this PR and also the configuration?

jaysonsantos · 2022-09-01T13:08:34Z

@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?

spacewander · 2022-09-02T06:01:16Z

@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

docs/en/latest/plugins/opentelemetry.md

tokers · 2022-09-02T09:28:54Z

apisix/plugins/opentelemetry.lua

+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)


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:

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.

spacewander · 2022-09-04T12:20:05Z

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


Could you move the dev kit stuff to a separate PR? Thanks!

i've dropped the commit

spacewander · 2022-09-04T12:20:44Z

apisix/plugins/opentelemetry.lua

@@ -169,6 +169,13 @@ local schema = {
                type = "string",
                minLength = 1,
            }
+        },
+        additional_header_attributes = {


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.

spacewander · 2022-09-04T12:27:07Z

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


key:sub(-1, -1)
Would be better to use :byte?

…x_attributes

* upstream/master: (214 commits) feat: set constants.apisix_lua_home for used by plugins (apache#7893) fix: response-rewrite plugin might cause Apache AIPSIX hanging (apache#7836) test: sleep 1 second in t/cli/test_upstream_mtls.sh (apache#7889) fix: reload once when log rotate (apache#7869) change: move etcd conf under deployment (apache#7860) fix: plugin metadata missing v3 adapter call (apache#7877) docs: add ClickHouse and Elasticsearch loggers. (apache#7848) docs(plugin): refactor limit-conn.md (apache#7857) feat: call `destroy` method when Nginx exits (apache#7866) feat: Add ability to inject headers via prefix to otel traces (apache#7822) docs(plugin): refactor proxy-cache.md (apache#7858) fix: don't enable passive healthcheck by default (apache#7850) feat: add openfunction plugin (apache#7634) fix(zipkin): send trace IDs with a reject sampling decision (apache#7833) fix: Change opentelemetry's span kind to server (apache#7830) docs(hmac-auth): additional details for generating signing_string (apache#7816) docs: correct the test-nginx description (apache#7818) docs: add docs of workflow plugin (apache#7813) docs(FAQ): add how to detect APISIX alive status (apache#7812) feat: add elasticsearch-logger (apache#7643) ...

…#7822) Co-authored-by: tzssangglass <tzssangglass@gmail.com>

feat: Add ability to inject headers to otel traces

1d269ee

tokers reviewed Aug 31, 2022

View reviewed changes

chore: Reuse string.has_prefix form apisix.core

429c718

jaysonsantos requested a review from tokers August 31, 2022 10:42

tzssangglass reviewed Aug 31, 2022

View reviewed changes

apisix/plugins/opentelemetry.lua Outdated Show resolved Hide resolved

tzssangglass reviewed Aug 31, 2022

View reviewed changes

apisix/plugins/opentelemetry.lua Outdated Show resolved Hide resolved

Apply suggestions from code review

07c13ba

Co-authored-by: tzssangglass <tzssangglass@gmail.com>

jaysonsantos requested review from tzssangglass and removed request for tokers August 31, 2022 11:59

chore: Change test output check to avoid editorconfig breaking

45c6d4a

spacewander reviewed Sep 1, 2022

View reviewed changes

jaysonsantos changed the title ~~feat: Add ability to inject headers to otel traces~~ feat: Add ability to inject headers via prefix to otel traces Sep 1, 2022

starsz reviewed Sep 2, 2022

View reviewed changes

docs/en/latest/plugins/opentelemetry.md Outdated Show resolved Hide resolved

tokers reviewed Sep 2, 2022

View reviewed changes

jaysonsantos requested review from spacewander and tokers and removed request for tzssangglass, spacewander and tokers September 2, 2022 12:40

spacewander reviewed Sep 4, 2022

View reviewed changes

jaysonsantos added 2 commits September 5, 2022 07:53

Apply code review suggestions

1d91fd3

chore: Extract asterisk logic to a constant and use key:byte

8266b2a

jaysonsantos force-pushed the add-ability-to-enrich-otel-with-headers branch from b5287fb to c82db59 Compare September 5, 2022 08:05

jaysonsantos requested a review from spacewander September 5, 2022 08:06

chore: Rename additional_header_attributes to additional_header_prefi…

a19a27b

…x_attributes

jaysonsantos force-pushed the add-ability-to-enrich-otel-with-headers branch from c82db59 to a19a27b Compare September 5, 2022 08:07

spacewander approved these changes Sep 6, 2022

View reviewed changes

starsz approved these changes Sep 7, 2022

View reviewed changes

spacewander merged commit 6cc1201 into apache:master Sep 7, 2022

jaysonsantos deleted the add-ability-to-enrich-otel-with-headers branch September 7, 2022 09:13

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022

feat: Add ability to inject headers via prefix to otel traces (apache…

2ce2a34

…#7822) Co-authored-by: tzssangglass <tzssangglass@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to inject headers via prefix to otel traces #7822

feat: Add ability to inject headers via prefix to otel traces #7822

jaysonsantos commented Aug 30, 2022

tokers Aug 31, 2022

spacewander left a comment

jaysonsantos commented Sep 1, 2022

spacewander commented Sep 2, 2022

tokers Sep 2, 2022

spacewander Sep 4, 2022

jaysonsantos Sep 5, 2022

spacewander Sep 4, 2022

spacewander Sep 4, 2022

feat: Add ability to inject headers via prefix to otel traces #7822

feat: Add ability to inject headers via prefix to otel traces #7822

Conversation

jaysonsantos commented Aug 30, 2022

Description

Checklist

tokers Aug 31, 2022

Choose a reason for hiding this comment

spacewander left a comment

Choose a reason for hiding this comment

jaysonsantos commented Sep 1, 2022

spacewander commented Sep 2, 2022

tokers Sep 2, 2022

Choose a reason for hiding this comment

spacewander Sep 4, 2022

Choose a reason for hiding this comment

jaysonsantos Sep 5, 2022

Choose a reason for hiding this comment

spacewander Sep 4, 2022

Choose a reason for hiding this comment

spacewander Sep 4, 2022

Choose a reason for hiding this comment