Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(correlation-id) new "tracker" generator #1288

Closed
wants to merge 8 commits into from
Closed

feat(correlation-id) new "tracker" generator #1288

wants to merge 8 commits into from

Conversation

WALL-E
Copy link
Contributor

@WALL-E WALL-E commented Jun 3, 2016

SEE ALSO #1228

@Tieske Tieske self-assigned this Jun 3, 2016
@Tieske
Copy link
Member

Tieske commented Jun 3, 2016

👍 great! code lgtm

What's left;

ngx.var.pid,
ngx.var.connection, -- connection serial number
ngx.var.connection_requests, -- current number of requests made through a connection
ngx.now() -- the current time stamp from the nginx cached time.
Copy link
Member

Choose a reason for hiding this comment

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

Could we cache all those global accesses? Something like:

local fmt = string.format
local now = ngx.now
local pid = ngx.worker.pid -- only need to retrieve it once

-- ...

local var = ngx.var
return fmt(...,
var.server_addr,
var.server_port,
pid,
-- ...
now()
)

@thibaultcha
Copy link
Member

I'm afraid the chances of collision is really high with this generator. ngx.now sure provides milliseconds precision but surely that won't be enough to guarantee a level of uniqueness as high as uuids.

@WALL-E
Copy link
Contributor Author

WALL-E commented Jun 3, 2016

updated doc is done.

@Tieske
Copy link
Member

Tieske commented Jun 3, 2016

I'm afraid the chances of collision is really high with this generator. ngx.now sure provides milliseconds precision but surely that won't be enough to guarantee a level of uniqueness as high as uuids.

yes, crossed my mind as well. But figured it would be easy to add a counter within the millisecond precision if needs be and it actually causes collisions. But maybe better to prevent it any way and add such a counter right away.

@WALL-E
Copy link
Contributor Author

WALL-E commented Jun 3, 2016

in modern operating system, pid_t is int type, and the pid will be increasing(pid%MAX_INT),
at the same time(milliseconds precision), I think the pid not repeat.

can you tell me that I'm wrong? @Tieske @thibaultcha

@Tieske
Copy link
Member

Tieske commented Jun 3, 2016

not sure, but I don't think you should be caching the magic table ngx on module level. Sen t you an update.

Not sure I understand your comment (with pid_t) but the question imo is whether we'll assume there will be no collisions, or we code to make sure there are none.

@timerickson
Copy link

And dont forget subsequent requests may come to different nodes on different hosts who may have varying clocks which could increase the chances of collision.

@WALL-E
Copy link
Contributor Author

WALL-E commented Jun 3, 2016

Correlation ID = "ip-port-pid-connection-connection_requests-timestamp"

ip: different hosts
port: different kong node
pid: different process (pid's type is pid_t, this number can be very large when increasing)
connection: serial number (different connection)
connection_requests: serial number on same connection (different request when HTTP Keepalive)
timestamp: milliseconds precision (different time)

code can be make sure there are no collisions,
but I'm not familiar with kong's cluster specifics, I'm not sure it is ok in the cluster.

ngx.worker.pid(),
ngx.var.connection, -- connection serial number
ngx.var.connection_requests, -- current number of requests made through a connection
ngx.now() -- the current time stamp from the nginx cached time.
Copy link
Member

Choose a reason for hiding this comment

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

I see you cached string.format, but please do follow my other caching suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I already gave it a quick edit; https://github.com/WALL-E/kong/pull/1

@thibaultcha
Copy link
Member

Right, the combination of $connection and $connection_requests should avoid any collision indeed. This is still missing a test.

@Tieske Tieske mentioned this pull request Jun 24, 2016
@thibaultcha thibaultcha changed the title Feat(plugins/Correlation ID) add new Generators feat(correlation-id) new "tracker" generator Jun 27, 2016
@@ -79,6 +82,12 @@ describe("Correlation ID Plugin", function()
end)
end)

describe("tracker genetator", function()
Copy link
Member

Choose a reason for hiding this comment

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

typo: generator

@Tieske
Copy link
Member

Tieske commented Jul 19, 2016

@WALL-E can you rebase on next? LGTM, except for the typo.

@thibaultcha any more comments, or ready to merge? (after rebase)

@thibaultcha
Copy link
Member

Manually applied the patch with minor modifications to next. Thank you for your contribution!

@kamoljan
Copy link

Looking forward to next release :)

@WALL-E
Copy link
Contributor Author

WALL-E commented Aug 9, 2016

I seem to have missed the beautiful moment :(

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.

5 participants