You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/02/26 03:43:58 UTC
[GitHub] [skywalking-nginx-lua] moonming opened a new pull request #3:
feature: add CI and test cases for ngx lua.
moonming opened a new pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] wu-sheng commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384318472
##########
File path: lib/skywalking/util.lua
##########
@@ -1,34 +1,92 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
+--
+local _M = {}
+
+-- for pure Lua
+local split = function(str, delimiter)
+ local t = {}
+ for substr in string.gmatch(str, "[^".. delimiter.. "]*") do
+ if substr ~= nil and string.len(substr) > 0 then
+ table.insert(t,substr)
+ end
+ end
+ return t
+end
+
+local timestamp = function()
+ local _, b = math.modf(os.clock())
+ if b==0 then
+ b='000'
+ else
+ b=tostring(b):sub(3,5)
+ end
+
+ return os.time() * 1000 + b
+end
+
+-- for Nginx Lua
+local ok, ngx_re = pcall(require, "ngx.re")
+if ok then
+ split = ngx_re.split
+ timestamp = function()
+ return ngx.now() * 1000
+ end
+end
+
+_M.split = split
+_M.timestamp = timestamp
-local Util = {}
local MAX_ID_PART2 = 1000000000
local MAX_ID_PART3 = 100000
-local SEQ = 1
+local metadata_buffer = ngx.shared.tracing_buffer
Review comment:
I think this is incompatible for pure lua.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] moonming commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384326023
##########
File path: lib/skywalking/segment_ref.lua
##########
@@ -1,20 +1,20 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
-
+--
+local ngx_re = require("ngx.re")
Review comment:
I forgot to fix here. I will add back.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] moonming commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384327785
##########
File path: lib/skywalking/util.lua
##########
@@ -1,34 +1,92 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
+--
+local _M = {}
+
+-- for pure Lua
+local split = function(str, delimiter)
+ local t = {}
+ for substr in string.gmatch(str, "[^".. delimiter.. "]*") do
+ if substr ~= nil and string.len(substr) > 0 then
+ table.insert(t,substr)
+ end
+ end
+ return t
+end
+
+local timestamp = function()
+ local _, b = math.modf(os.clock())
+ if b==0 then
+ b='000'
+ else
+ b=tostring(b):sub(3,5)
+ end
+
+ return os.time() * 1000 + b
+end
+
+-- for Nginx Lua
+local ok, ngx_re = pcall(require, "ngx.re")
+if ok then
+ split = ngx_re.split
+ timestamp = function()
+ return ngx.now() * 1000
+ end
+end
+
+_M.split = split
+_M.timestamp = timestamp
-local Util = {}
local MAX_ID_PART2 = 1000000000
local MAX_ID_PART3 = 100000
-local SEQ = 1
+local metadata_buffer = ngx.shared.tracing_buffer
Review comment:
I will remove 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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] wu-sheng commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384317008
##########
File path: lib/skywalking/segment_ref.lua
##########
@@ -1,20 +1,20 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
-
+--
+local ngx_re = require("ngx.re")
Review comment:
Do you miss the change here? `segment_ref` is a core related part.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] wu-sheng commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384319371
##########
File path: lib/skywalking/util.lua
##########
@@ -1,34 +1,92 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
+--
+local _M = {}
+
+-- for pure Lua
+local split = function(str, delimiter)
+ local t = {}
+ for substr in string.gmatch(str, "[^".. delimiter.. "]*") do
+ if substr ~= nil and string.len(substr) > 0 then
+ table.insert(t,substr)
+ end
+ end
+ return t
+end
+
+local timestamp = function()
+ local _, b = math.modf(os.clock())
+ if b==0 then
+ b='000'
+ else
+ b=tostring(b):sub(3,5)
+ end
+
+ return os.time() * 1000 + b
+end
+
+-- for Nginx Lua
+local ok, ngx_re = pcall(require, "ngx.re")
+if ok then
+ split = ngx_re.split
+ timestamp = function()
+ return ngx.now() * 1000
+ end
+end
+
+_M.split = split
+_M.timestamp = timestamp
-local Util = {}
local MAX_ID_PART2 = 1000000000
local MAX_ID_PART3 = 100000
-local SEQ = 1
+local metadata_buffer = ngx.shared.tracing_buffer
-function Util:newID()
- SEQ = SEQ + 1
- return {Util.timestamp(), math.random( 0, MAX_ID_PART2), math.random( 0, MAX_ID_PART3) + SEQ}
+local random_seed = function ()
+ local seed
+ local frandom = io.open("/dev/urandom", "rb")
+ if frandom then
+ local str = frandom:read(4)
+ frandom:close()
+ if str then
+ local s = 0
+ for i = 1, 4 do
+ s = 256 * s + str:byte(i)
+ end
+ seed = s
+ end
+ end
+
+ if not seed then
+ seed = ngx.now() * 1000 + ngx.worker.pid()
+ end
+
+ return seed
+end
+
+math.randomseed(random_seed())
+
+function _M.newID()
Review comment:
The ID algorithm should be good enough even without seq. `ngx.shared.tracing_buffer` force them to increase strictly, which is not necessary. In other language, it just provided a static int to do `i++`. If this is hard in Lua or nginx lua case, feel free to delete this `seq`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] wu-sheng merged pull request #3: feature:
add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] wu-sheng commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384317263
##########
File path: lib/skywalking/segment_ref.lua
##########
@@ -1,20 +1,20 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
-
+--
+local ngx_re = require("ngx.re")
Review comment:
I have read there is a compatible in util.lua.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] wu-sheng commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384319605
##########
File path: t/util.t
##########
@@ -0,0 +1,96 @@
+use Test::Nginx::Socket 'no_plan';
Review comment:
What is this file for? Could you give me some guides?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] moonming commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384326679
##########
File path: lib/skywalking/util.lua
##########
@@ -1,34 +1,92 @@
---
+--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
---
+--
-- http://www.apache.org/licenses/LICENSE-2.0
---
+--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
---
+--
+local _M = {}
+
+-- for pure Lua
+local split = function(str, delimiter)
+ local t = {}
+ for substr in string.gmatch(str, "[^".. delimiter.. "]*") do
+ if substr ~= nil and string.len(substr) > 0 then
+ table.insert(t,substr)
+ end
+ end
+ return t
+end
+
+local timestamp = function()
+ local _, b = math.modf(os.clock())
+ if b==0 then
+ b='000'
+ else
+ b=tostring(b):sub(3,5)
+ end
+
+ return os.time() * 1000 + b
+end
+
+-- for Nginx Lua
+local ok, ngx_re = pcall(require, "ngx.re")
+if ok then
+ split = ngx_re.split
+ timestamp = function()
+ return ngx.now() * 1000
+ end
+end
+
+_M.split = split
+_M.timestamp = timestamp
-local Util = {}
local MAX_ID_PART2 = 1000000000
local MAX_ID_PART3 = 100000
-local SEQ = 1
+local metadata_buffer = ngx.shared.tracing_buffer
-function Util:newID()
- SEQ = SEQ + 1
- return {Util.timestamp(), math.random( 0, MAX_ID_PART2), math.random( 0, MAX_ID_PART3) + SEQ}
+local random_seed = function ()
+ local seed
+ local frandom = io.open("/dev/urandom", "rb")
+ if frandom then
+ local str = frandom:read(4)
+ frandom:close()
+ if str then
+ local s = 0
+ for i = 1, 4 do
+ s = 256 * s + str:byte(i)
+ end
+ seed = s
+ end
+ end
+
+ if not seed then
+ seed = ngx.now() * 1000 + ngx.worker.pid()
+ end
+
+ return seed
+end
+
+math.randomseed(random_seed())
+
+function _M.newID()
Review comment:
got it. `seq` is hard to implement in ngx lua, I will remove `seq`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [skywalking-nginx-lua] moonming commented on a change in pull
request #3: feature: add CI and test cases for ngx lua.
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #3: feature: add CI and test cases for ngx lua.
URL: https://github.com/apache/skywalking-nginx-lua/pull/3#discussion_r384327598
##########
File path: t/util.t
##########
@@ -0,0 +1,96 @@
+use Test::Nginx::Socket 'no_plan';
Review comment:
It's test framework in Nginx lua: https://metacpan.org/pod/Test::Nginx::Socket, most of nginx lua module are test by `test::nginx`.
----------------------------------------------------------------
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
With regards,
Apache Git Services