-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(opentelemetry): add opentelemetry variables #8871
feat(opentelemetry): add opentelemetry variables #8871
Conversation
@starsz @yangxikun @shreemaan-abhishek Please take a look when you have free time, thanks! |
missing test cases @lework |
```yaml title="./conf/config.yaml" | ||
nginx_config: | ||
http_server_configuration_snippet: | | ||
set $opentelemetry_context_traceparent "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I think there is another way that we can add those lines in cli/ngx_tpl.lua
automatically if we enabled the "OpenTelemetry" plugins.
Else APISIX will panic the request is we forget to set those line but enable the set_ngx_var
options in the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I added some code
@starsz That ci error above looks like an error reported by another plugin, what should I do? |
Looks strange. Can you merge the master branch and try again? |
@membphis Looking for help on what I should do next? |
@monkeyDluffy6017 please review this PR |
@monkeyDluffy6017 @moonming please review this PR,It's been too long. |
We'd also need this before migrating to apisix. |
@monkeyDluffy6017 any update? |
Nice work! |
set_ngx_var = { | ||
type = "boolean", | ||
description = "set nginx variables", | ||
default = false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_ngx_var = { | |
type = "boolean", | |
description = "set nginx variables", | |
default = false, | |
}, | |
set_ngx_var = { | |
type = "boolean", | |
description = "set nginx variables", | |
default = false, | |
}, |
ngx_var.opentelemetry_context_traceparent = string_format("00-%s-%s-%02x", | ||
span_context.trace_id, | ||
span_context.span_id, | ||
span_context.trace_flags) | ||
ngx_var.opentelemetry_trace_id = span_context.trace_id | ||
ngx_var.opentelemetry_span_id = span_context.span_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to use ctx.var
instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest LGTM.
Description
Span information recorded in the access log.
Fixes # #8765
Checklist