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

fix: adjust the execution priority of request-id to fix opentelemetry has no request id #7281

Merged
merged 7 commits into from
Jun 22, 2022

Conversation

jagerzhang
Copy link
Contributor

@jagerzhang jagerzhang commented Jun 20, 2022

Description

Adjust the execution priority of request-id from 11010 to 12015

...
  - request-id                     # priority: 12015
  - zipkin                            # priority: 12011
  #- skywalking                  # priority: 12010
  #- opentelemetry            # priority: 12009
...

Fixes #7239

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)

@jagerzhang jagerzhang changed the title feat: adjust the execution priority after the request-id fix: adjust the execution priority after the request-id Jun 20, 2022
@@ -176,7 +176,7 @@ local schema = {

local _M = {
version = 0.1,
priority = 12009,
priority = 11009,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not a good idea to set it adjacent to the request-id plugin. It's not extendable if we need to insert another plugin between them in the future.

Copy link
Contributor Author

@jagerzhang jagerzhang Jun 20, 2022

Choose a reason for hiding this comment

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

I think it's not a good idea to set it adjacent to the request-id plugin. It's not extendable if we need to insert another plugin between them in the future.

OK, already change to 11005

@tzssangglass
Copy link
Member

we also need to update the config-default.yaml

@tokers
Copy link
Contributor

tokers commented Jun 21, 2022

@jagerzhang Also, you need to check out the CI failures, we have some test cases to cover the plugin priority settings.

@jagerzhang jagerzhang changed the title fix: adjust the execution priority after the request-id fix: adjust the execution priority of request-id to fix opentelemetry has no request id Jun 21, 2022
@jagerzhang
Copy link
Contributor Author

@tokers
CI report this error, I don't know why this file not found.

Error: [error] [lua] config_yaml.lua:72: read_apisix_yaml(): failed to fetch /apisix/t/servroot/conf/apisix.yaml attributes: cannot obtain information from file '/apisix/t/servroot/conf/apisix.yaml': No such file or directory

@tokers
Copy link
Contributor

tokers commented Jun 21, 2022

@tokers CI report this error, I don't know why this file not found.

Error: [error] [lua] config_yaml.lua:72: read_apisix_yaml(): failed to fetch /apisix/t/servroot/conf/apisix.yaml attributes: cannot obtain information from file '/apisix/t/servroot/conf/apisix.yaml': No such file or directory

Looks like this is the actual error: https://github.com/apache/apisix/runs/6977549863?check_suite_focus=true#step:15:700

@jagerzhang
Copy link
Contributor Author

jagerzhang commented Jun 22, 2022

@tokers I found that the order of plugins will verified in many places, like:

loaded plugin and sort by priority: 23000 name: real-ip
loaded plugin and sort by priority: 22000 name: client-control
loaded plugin and sort by priority: 12011 name: zipkin
loaded plugin and sort by priority: 12000 name: ext-plugin-pre-req
loaded plugin and sort by priority: 11010 name: request-id
loaded plugin and sort by priority: 11000 name: fault-injection
loaded plugin and sort by priority: 10000 name: serverless-pre-function
loaded plugin and sort by priority: 4000 name: cors
loaded plugin and sort by priority: 3000 name: ip-restriction
loaded plugin and sort by priority: 2990 name: referer-restriction
loaded plugin and sort by priority: 2900 name: uri-blocker
loaded plugin and sort by priority: 2800 name: request-validation
loaded plugin and sort by priority: 2599 name: openid-connect
loaded plugin and sort by priority: 2555 name: wolf-rbac
loaded plugin and sort by priority: 2530 name: hmac-auth
loaded plugin and sort by priority: 2520 name: basic-auth
loaded plugin and sort by priority: 2510 name: jwt-auth
loaded plugin and sort by priority: 2500 name: key-auth
loaded plugin and sort by priority: 2400 name: consumer-restriction
loaded plugin and sort by priority: 2000 name: authz-keycloak
loaded plugin and sort by priority: 1010 name: proxy-mirror
loaded plugin and sort by priority: 1009 name: proxy-cache
loaded plugin and sort by priority: 1008 name: proxy-rewrite
loaded plugin and sort by priority: 1005 name: api-breaker
loaded plugin and sort by priority: 1003 name: limit-conn
loaded plugin and sort by priority: 1002 name: limit-count
loaded plugin and sort by priority: 1001 name: limit-req
loaded plugin and sort by priority: 995 name: gzip
loaded plugin and sort by priority: 990 name: server-info
loaded plugin and sort by priority: 966 name: traffic-split
loaded plugin and sort by priority: 900 name: redirect
loaded plugin and sort by priority: 899 name: response-rewrite
loaded plugin and sort by priority: 506 name: grpc-transcode
loaded plugin and sort by priority: 500 name: prometheus
loaded plugin and sort by priority: 412 name: echo
loaded plugin and sort by priority: 410 name: http-logger
loaded plugin and sort by priority: 406 name: sls-logger
loaded plugin and sort by priority: 405 name: tcp-logger
loaded plugin and sort by priority: 403 name: kafka-logger
loaded plugin and sort by priority: 402 name: rocketmq-logger
loaded plugin and sort by priority: 401 name: syslog
loaded plugin and sort by priority: 400 name: udp-logger
loaded plugin and sort by priority: 398 name: clickhouse-logger
loaded plugin and sort by priority: 0 name: example-plugin
loaded plugin and sort by priority: -2000 name: serverless-post-function
loaded plugin and sort by priority: -3000 name: ext-plugin-post-req

https://github.com/apache/apisix/blob/master/t/admin/plugins.t#L63-L131
https://github.com/apache/apisix/blob/master/conf/config-default.yaml#L335-L410

I think it may be more convenient to uniformly abstract to one place, such as uniformly reading from config-default yaml.

@jagerzhang
Copy link
Contributor Author

@spacewander spacewander merged commit 291824f into apache:master Jun 22, 2022
spacewander pushed a commit that referenced this pull request Jun 30, 2022
… has no request id (#7281)

Co-authored-by: Jagerzhang <jagerzhang@tencent.com>
spacewander pushed a commit to spacewander/incubator-apisix that referenced this pull request Aug 17, 2022
… has no request id (apache#7281)

Co-authored-by: Jagerzhang <jagerzhang@tencent.com>
spacewander pushed a commit that referenced this pull request Aug 17, 2022
… has no request id (#7281)

Co-authored-by: Jagerzhang <jagerzhang@tencent.com>
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
… has no request id (apache#7281)

Co-authored-by: Jagerzhang <jagerzhang@tencent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

help request: 插件 request-id 与 opentelemetry 结合
5 participants