Skip to content
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

Merged
merged 10 commits into from
Sep 21, 2023

Conversation

lework
Copy link
Contributor

@lework lework commented Feb 16, 2023

Description

Span information recorded in the access log.

Fixes # #8765

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)

@lework lework mentioned this pull request Feb 16, 2023
5 tasks
@lework
Copy link
Contributor Author

lework commented Feb 28, 2023

@starsz @yangxikun @shreemaan-abhishek Please take a look when you have free time, thanks!

@membphis
Copy link
Member

missing test cases @lework

```yaml title="./conf/config.yaml"
nginx_config:
http_server_configuration_snippet: |
set $opentelemetry_context_traceparent ""
Copy link
Contributor

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.

Copy link
Contributor Author

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

@lework
Copy link
Contributor Author

lework commented Mar 21, 2023

@starsz That ci error above looks like an error reported by another plugin, what should I do?

@starsz
Copy link
Contributor

starsz commented Mar 22, 2023

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

@lework
Copy link
Contributor Author

lework commented Mar 30, 2023

@membphis Looking for help on what I should do next?

starsz
starsz previously approved these changes Jun 5, 2023
@moonming
Copy link
Member

@monkeyDluffy6017 please review this PR

@lework
Copy link
Contributor Author

lework commented Aug 24, 2023

@monkeyDluffy6017 @moonming please review this PR,It's been too long.

@indrekj
Copy link
Contributor

indrekj commented Sep 21, 2023

We'd also need this before migrating to apisix.

@moonming
Copy link
Member

@monkeyDluffy6017 any update?

@monkeyDluffy6017
Copy link
Contributor

Nice work!

Comment on lines +116 to +120
set_ngx_var = {
type = "boolean",
description = "set nginx variables",
default = false,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set_ngx_var = {
type = "boolean",
description = "set nginx variables",
default = false,
},
set_ngx_var = {
type = "boolean",
description = "set nginx variables",
default = false,
},

Comment on lines +344 to +349
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
Copy link
Member

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 ?

Copy link
Member

@leslie-tsang leslie-tsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM.

@monkeyDluffy6017 monkeyDluffy6017 merged commit 6bf6ab9 into apache:master Sep 21, 2023
40 checks passed
@moonming
Copy link
Member

@lework, thanks for your contribution 👍
@indrekj Welcome to continue migrating to Apache APISIX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants