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 2020/11/12 14:12:00 UTC
[GitHub] [apisix] gy09535 opened a new pull request #2727: feat: Improve zipkin span reporter performance
gy09535 opened a new pull request #2727:
URL: https://github.com/apache/apisix/pull/2727
### What this PR does / why we need it:
fix: https://github.com/apache/apisix/issues/2726
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Miss-you commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
Miss-you commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-726203323
Please add some automated test cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526127630
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@spacewander After check I find use "cjson" here can not be cause exception, for encode just "function"
"ightuserdata(not null value)" "thread" can cause exception, anyway this entity is table, use cjson.safe equals use cjson. I think use cjson is ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r524934920
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
agreed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522866826
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
Review comment:
for test case, I will change it .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r523867234
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,39 +105,71 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res, err = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ core.log.error("report span error: ", err)
+ return nil, "failed: " .. report.endpoint
Review comment:
Better to pass the `err` in the returned error message, so that we can log it together.
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
Maybe we can put `cjson.encode_number_precision(16)` into `core.json`? CC @moonming
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-727695044
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525329993
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,39 +105,72 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
Review comment:
Two blank lines are required here.
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,39 +105,72 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res, err = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ -- for zipkin test
+ core.log.error("report zipkin span failed")
+ return nil, "failed: " .. err .. ", url: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
- return nil, "failed: " .. res.status .. " " .. res.reason
+ return nil, "failed: " .. report.endpoint .. " "
+ .. res.status .. " " .. res.reason
end
return true
end
Review comment:
ditto.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526181235
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
And, `this entity must be table` doesn't mean ` can not be cause exception`. You can see my example above:
```lua
local cjson = require("cjson")
local x = cjson.new()
local t = {}
t[1] = 1
t[1000] = 1
print(x.encode({a = t}))
-- ERROR: /tmp/a.lua:7: Cannot serialise table: excessively sparse array
```
Please! Please! Please! Be careful and never say something bad won't happen. I am tired of telling the same thing too many times.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522842060
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
Review comment:
Why don't check the error?
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
+ else
+ pending_spans, err = cjson.encode(entries)
Review comment:
Style: indent
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
Better to switch to `core.json`.
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
Review comment:
Style: indent
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522884746
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
Review comment:
fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-727920569
Please confirm @moonming .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526545706
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,39 +105,72 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res, err = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ -- for zipkin test
+ core.log.error("report zipkin span failed")
+ return nil, "failed: " .. err .. ", url: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
- return nil, "failed: " .. res.status .. " " .. res.reason
+ return nil, "failed: " .. report.endpoint .. " "
+ .. res.status .. " " .. res.reason
end
return true
end
Review comment:
fixed
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,39 +105,72 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
Review comment:
fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r524934920
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
sorry, I changed my mind, I think keep `encode_number_precision(16)` in zipkin is ok, others not need so long id.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525945492
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@gy09535
I don't think so. A proper error handling is better than an unexpected error. In proper error handling we can log the ctx, but we can't do the same for an unexpected error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522963223
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
I think it maybe the trace Id is to long.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522880514
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
I am not sure this code "cjson.encode_number_precision(16)" can affect encoding.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 edited a comment on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 edited a comment on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-729452331
For code style should we have some tools such vs code format config?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525832876
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
I think it doesn't matter in this case, it is a batch job and can not be has error, because for input spans must be serialized.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526181235
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
And, `this entity must be table` doesn't mean ` can not be cause exception`. You can see my example above:
```lua
local cjson = require("cjson")
local x = cjson.new()
local t = {}
t[1] = 1
t[1000] = 1
print(x.encode({a = t}))
# ERROR: /tmp/a.lua:7: Cannot serialise table: excessively sparse array
```
Please! Please! Please! Be careful and never say something bad won't happen. I am tired of telling the same thing too many times.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522884580
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
+ else
+ pending_spans, err = cjson.encode(entries)
Review comment:
fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526127630
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@spacewander After check I find use "cjson" here can not be cause exception, this entity must be table, for encoding use table is ok, use cjson.safe equals use cjson. I think use cjson is ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #2727:
URL: https://github.com/apache/apisix/pull/2727
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525832876
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
I think it doesn't matter in this case, it is a batch job and can not be has error, because for input spans must be serialized.If it can not be serialized, must be a bug?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525757548
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@moonming @gy09535
I suddenly realized one thing: since `core.json` use `cjson` inside, when we set `encode_number_precision(16)`, we already do the same thing for `cjson`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-726575429
> Please add some automated test cases.
Added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526161951
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@gy09535
In similar code of batch processor, we always check the error.
It is meaningless to check the error, if you insist on using `cjson` directly, since the `encode` method from this module never returns `nil, err`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r526545586
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
Got it, fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-729452331
For code style should we has some tools such vs code format config?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-729715738
> > For code style should we have some tools such vs code format config?
>
> I've been looking for it, but I didn't find a useful one.
Got it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525627348
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@gy09535
Therefore we need to use `cjson.safe` instead. Otherwise an error will be thrown when encoding failed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r522884439
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
Review comment:
fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r523924761
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,39 +105,71 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res, err = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ core.log.error("report span error: ", err)
+ return nil, "failed: " .. report.endpoint
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gy09535 commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525832876
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
I think it doesn't matter in this case, it is a batch job and can not be has error, because for input spans must be serialized.If it can not be serialized, must be bug?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525757548
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@moonming @gy09535
I suddenly realized one thing: since `core.json` use `cjson` inside, when we set `encode_number_precision(16)`, we already do the same thing for `cjson`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525945492
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
+ local res = httpc:request_uri(report.endpoint, {
method = "POST",
headers = {
["content-type"] = "application/json",
},
body = pending_spans,
+ keepalive = 5000,
+ keepalive_pool = 5
})
- -- TODO: on failure, retry?
if not res then
- return nil, "failed to request: " .. err
+ return nil, "failed: " .. report.endpoint
elseif res.status < 200 or res.status >= 300 then
return nil, "failed: " .. res.status .. " " .. res.reason
end
- return true
+ return true
+end
+
+function _M.init_processor(self)
+ local process_conf = {
+ name = "zipkin_report",
+ retry_delay = 1,
+ batch_max_size = 1000,
+ max_retry_count = 0,
+ buffer_duration = 60,
+ inactive_timeout = 5
+ }
+
+ local flush = function (entries, batch_max_size)
+ if not entries then
+ return true
+ end
+
+ local pending_spans, err
+ if batch_max_size == 1 then
+ pending_spans, err = cjson.encode(entries[1])
Review comment:
@gy09535
I don't think so. A proper error handling is better than an unexpected error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#discussion_r525944847
##########
File path: apisix/plugins/zipkin/reporter.lua
##########
@@ -105,38 +104,68 @@ function _M.report(self, span)
annotations = span.logs
}
- local i = self.pending_spans_n + 1
- self.pending_spans[i] = zipkin_span
- self.pending_spans_n = i
-end
-
-function _M.flush(self)
- if self.pending_spans_n == 0 then
-
- return true
+ self.pending_spans_n = self.pending_spans_n + 1
+ if self.processor then
+ self.processor:push(zipkin_span)
end
+end
- local pending_spans = cjson.encode(self.pending_spans)
- self.pending_spans = {}
- self.pending_spans_n = 0
-
+local function send_span(pending_spans, report)
local httpc = resty_http.new()
- local res, err = httpc:request_uri(self.endpoint, {
Review comment:
@spacewander please check it, this PR is waiting your approve
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2727: feat: Improve zipkin span reporter performance
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2727:
URL: https://github.com/apache/apisix/pull/2727#issuecomment-729506467
> For code style should we have some tools such vs code format config?
I've been looking for it, but I didn't find a useful one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org