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

generate unique identifier for each request and pass to upsteram as a header #1086

Closed
ahmadnassri opened this issue Mar 17, 2016 · 19 comments
Closed
Labels
good first issue Issues that beginners/volunteers can easily help with.

Comments

@ahmadnassri
Copy link
Contributor

being able to trace requests coming through Kong with some sort of unique identifier would be helpful for further tracking in the upstream, a suggested header name can be: Kong-Request-ID (in accordance with #324)

@thibaultcha
Copy link
Member

This would be the easiest plugin to make. Beginners welcome.

@thibaultcha thibaultcha added the good first issue Issues that beginners/volunteers can easily help with. label Mar 17, 2016
@Tieske
Copy link
Member

Tieske commented Mar 17, 2016

any requirements for the ID? The easy answer is uuid, but that is slow. Will something like 'host-name + WorkerPID + counter' do?

@thibaultcha
Copy link
Member

The most important requirement would be true, guaranteed uniqueness, at least for some platforms which heavily rely on such a feature (for example tracking a money transaction from its request id). This could be configurable. Also if we are worried about speed, we could benchmark a uuid C binding vs an FFI implementation.

On Mar 17, 2016, at 5:40 PM, Thijs Schreijer [email protected] wrote:

any requirements for the ID? The easy answer is uuid, but that is slow. Will something like 'host-name + WorkerPID + counter' do?


You are receiving this because you commented.
Reply to this email directly or view it on GitHub

@Tieske
Copy link
Member

Tieske commented Mar 17, 2016

The most important requirement would be true, guaranteed uniqueness,

which requires good random algorithms, which are not cheap

How about generating a uuid once per worker process and append a counter value.

@thibaultcha
Copy link
Member

I could see that work, it is also nice because since it is a per-worker state, the counter can simply live as a non-global in the module.
The trade off is on the user side, where storage gets a bit more complicated and querying/indexing potentially harder (pure uuid vs string). I think it's minor though.
Kong aims at being as less opinionated as possible, so I think providing both options would be very easy and accommodate most of the use cases.

@ahmadnassri
Copy link
Contributor Author

Kong aims at being as less opinionated as possible

👍

could be two different plugins too, user can pick what level of performance / accuracy they want by making choice of plugin to use. (this true to any existing or future plugins)

@opyate
Copy link
Contributor

opyate commented Mar 18, 2016

Re: UUID, I've seen this (a C impl), this and this (pure Lua), and this and this (ffi).

Presuming Kong's lua_uuid is good enough...:

I would like to discuss how one might configure the plugin. This is to get stylistic issues out of the way before I start hacking at a POC.

Example 1

{
  "config": {
    "header_name": "Kong-Request-ID",
    "generator": "uuid"
  }
}
  1. config.header_name: Following @ahmadnassri 's suggestion of using Kong-Request-ID as the default header name, this could be a default in schema.lua.
  2. config.generator: It will default to the UUID generator. Other generators can include timestamp and @Tieske 's suggestions of host, pid, and counter and combinations of these.

Now the big question: how to specify combinations of generators, and especially sub-options, like "I only want one UUID per worker".

Example 2

{
  "config": {
    "header_name": "Kong-Request-ID",
    "generator": {
      "uuid": {
        "once-per-worker": true
      },
      "counter": {},
      "delimiter": "."
    }
  }
}
  1. config.generator.<generator-name>: are the settings for each generator, e.g. config.generator.uuid.once-per-worker set to true follows @Tieske 's "How about generating a uuid once per worker " suggestion.
  2. Example 2's generator config will generate a Kong-Request-ID with the value of a UUID (once per worker), followed by a dot, followed by a number as generated by counter, e.g. "80098e50-ed40-11e5-a807-27bda77c52c8.1337" for the 1337th request on said worker.

This is where I'd especially like feedback: counter is empty. Would it be better to specify generator as "{uuid:per-worker}-{counter}" and then parse it? Or do we stay away from DSLs? Other DSL tokens could be per-server or value:1337 for static things.

Example 3

{
  "config": {
    "header_name": "Kong-Request-ID",
    "generator": "{uuid:per-worker}.{counter}"
  }
}

So, to recap: I'm trying to think of this from the point of view of the administrator configuring the plugin, so I'd like feedback before putting pen to paper.

Lastly, a correlation-id works better with logging, so in the spirit of the Unix philosophy one can just suggest this plugin be used with one of the logging plugins so you can send Kong-Request-ID upstream AND see it in the logs, you know, for "correlation" purposes :)

@thibaultcha
Copy link
Member

Nothing too fancy. "UUID" and "UUID-counter" are enough. Not sure about the naming for the second one but it doesn't matter much.

As for the format, something like "uuid#counter" is probably suitable. It would be easier to parse for users than a dash like uuid's, so one can distinguish the worker's uuid from the counter easily.

@ajayk
Copy link
Contributor

ajayk commented Mar 18, 2016

@ahmadnassri @thibaultcha if there is an header [Kong-Request-ID] already present, the plugin should pass the Kong-Request-ID as is without modification , this would help in tracing api to api calls .

@opyate
Copy link
Contributor

opyate commented Mar 18, 2016

As for the format, something like "uuid#counter" is probably suitable. It would be easier to parse for users than a dash like uuid's, so one can distinguish the worker's uuid from the counter easily.

Yes, this would be user-specified, e.g.: curl ... --data 'config.generator=uuid;counter for semi-colon or curl ... --data 'config.generator=uuid#counter for hash.

@ajayk Noted!

@thibaultcha
Copy link
Member

No I think you are over complicating this though...

@Tieske
Copy link
Member

Tieske commented Mar 18, 2016

I agree, looks to complicated to me too.

Just an option to switch between a uuid-per-request or a uuid-per-worker+timestamp/counter should do.

@opyate
Copy link
Contributor

opyate commented Mar 19, 2016

Sure, with a hard-coded # delimiter, then.

So, to confirm, the user of the plugin has 2 options when choosing a generator:

For "unique uuid for every request":

--data 'config.generator=uuid'

For "unique uuid per worker process, with a counter, delimited by a #":

--data 'config.generator=uuid#counter'

@opyate
Copy link
Contributor

opyate commented Mar 21, 2016

All, please review my latest changes. I've made a note in my tests about relying on one worker process for some things.

Once you're happy, I'll squash and PR.

@Tieske
Copy link
Member

Tieske commented Mar 21, 2016

Code lgtm generally, some minor nit picks. But the changes to kong/tools/database_cache.lua can/should be reverted no?

@opyate
Copy link
Contributor

opyate commented Mar 21, 2016

I've reverted the changes to the cache, and will look at the commit comments, thank you.

@thibaultcha
Copy link
Member

Can you please open a PR so the review process is easier and centralized? Thanks.

@opyate
Copy link
Contributor

opyate commented Mar 22, 2016

@thibaultcha Done: #1094

opyate added a commit to opyate/kong that referenced this issue Mar 23, 2016
thibaultcha pushed a commit that referenced this issue Apr 11, 2016
Impement request tracking via headers through UUIDs and a combination of
UUIDs and worker ids for performance concerns.

fix #1086

Signed-off-by: Thibault Charbonnier <[email protected]>
@thibaultcha
Copy link
Member

Just manually merged the patch from #1094. Now applying the documentation. Thanks a lot!

We'll try to include this in 0.8 very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that beginners/volunteers can easily help with.
Projects
None yet
Development

No branches or pull requests

5 participants