correlation-id plugin, fixes #1086 #1094

opyate · 2016-03-22T19:30:45Z

No description provided.

thibaultcha · 2016-03-22T19:38:18Z

kong/plugins/correlation-id/handler.lua

+  end
+
+  -- For later use, to echo it back downstream
+  ngx.ctx.correlationid_header_value = header_value


I am not sure this feature benefits the plugin. I would expect such a behavior to be quite private and only related to the internals of one's infrastructure. I would be in favor of making this behavior configurable, and disabled by default.

Echoing back downstream?
Sure it will - it will help the caller "correlate" if there was a problem with the request.
E.g. mockbin also echos x-request-id back:

curl http://mockbin.com/request --data 'foo=bar' { "startedDateTime": "2016-03-22T19:42:26.954Z", "clientIPAddress": "77.101.29.24", "method": "POST", "url": "http://mockbin.com/request", "httpVersion": "HTTP/1.1", "cookies": {}, "headers": { "host": "mockbin.com", "connection": "close", "accept-encoding": "gzip", "x-forwarded-for": "77.101.29.24, 162.158.152.161", "cf-ray": "287c2079a0a73578-LHR", "x-forwarded-proto": "http", "cf-visitor": "{\"scheme\":\"http\"}", "user-agent": "curl/7.47.1", "accept": "*/*", "x-request-id": "b04bad2e-c240-43ad-a0f9-60c98d86599d", "content-type": "application/x-www-form-urlencoded", "cf-connecting-ip": "77.101.29.24", "x-forwarded-port": "80", "via": "1.1 vegur", "connect-time": "0", "x-request-start": "1458675746943", "total-route-time": "0", "content-length": "7" }, "queryString": {}, "postData": { "mimeType": "application/x-www-form-urlencoded", "text": "foo=bar", "params": { "foo": "bar" } }, "headersSize": 535, "bodySize": 7 }

Please confirm if you disagree, then I'll make it configurable.

Yes, it could help for sure, but I think most providers do not want to expose their internals to the public facing side of their services, is what I meant :)

(mockbin echoes it back because it simply echoes whatever you send to it)

thibaultcha · 2016-03-22T19:42:01Z

Hey, that looks great! Just added a few concerns. Thanks :)

coveralls · 2016-03-22T19:42:23Z

Coverage remained the same at 59.451% when pulling ed761a5edb618dbfda37a76c696b26cb81b34107 on opyate:correlation-id into c9d3e56 on Mashape:next.

thibaultcha · 2016-03-22T19:45:05Z

The linting fails with 2 errors:

$ make lint
Checking kong/plugins/oauth2/api.lua              1 warning
    kong/plugins/oauth2/api.lua:2:7: unused variable utils
Checking spec/plugins/oauth2/access_spec.lua      1 warning
    spec/plugins/oauth2/access_spec.lua:304:33: unused variable headers
Total: 2 warnings / 0 errors in 274 files

opyate · 2016-03-22T19:50:45Z

Not my code :)

thibaultcha · 2016-03-22T19:51:24Z

Oh indeed! I'm sorry haha, I should have checked! (not sure how it went through, I assumed that was not possible...)

ahmadnassri · 2016-03-22T19:59:23Z

yes, mockbin does its own x-request-id :)

opyate · 2016-03-22T20:00:30Z

I committed the spec matcher, but since rebasing this error popped up which means I don't know if my commit will work.

./spec/spec_helpers.lua:222: Error during migration 2016-03-07-jwt-alg: [Invalid] Invalid column name algorithm because it conflicts with an existing column

thibaultcha · 2016-03-22T20:00:55Z

yes, mockbin does its own x-request-id :)

Fair enough! But mockbin is a debugging/development tool :) I think we can all agree on why this should not be the default behavior for our use case.

opyate · 2016-03-22T20:06:49Z

OK, I'll make "echo correlation ID back to downstream" configurable.
Thanks for all the suggestions thus far!

coveralls · 2016-03-22T20:12:08Z

Coverage remained the same at 59.451% when pulling d612dc031b2e8402297b251624ec656136e32263 on opyate:correlation-id into c9d3e56 on Mashape:next.

thibaultcha · 2016-03-22T20:14:12Z

./spec/spec_helpers.lua:222: Error during migration 2016-03-07-jwt-alg: [Invalid] Invalid column name algorithm because it conflicts with an existing column

Fixing this is easy: delete all keyspaces in your Cassandra cluster (kong_development, kong, kong_tests) and make sure to be up-to-date with next.

opyate · 2016-03-22T20:23:30Z

sudo make clean and sudo make dev tripped up on JWT again, so I deleted all traces of it (temporarily for testing purposes), but now there's a new problem:

[----------] Running tests from spec/plugins/correlation-id/access_spec.lua
./kong/tools/faker.lua:87: Faker failed to insert api entity: upstream_url=http://mockbin.com request_host=correlation1.com
[Invalid] unconfigured columnfamily apis

stack traceback:
        ./kong/tools/faker.lua:87: in function 'insert_fixtures'
        spec/plugins/correlation-id/access_spec.lua:14: in function <spec/plugins/correlation-id/access_spec.lua:12>

./spec/spec_helpers.lua:75: spec_helper cannot stop kong: 
[INFO] Using configuration: kong_TEST.yml[ERR] Kong is not running

stack traceback:
        ./spec/spec_helpers.lua:75: in function 'stop_kong'
        spec/plugins/correlation-id/access_spec.lua:30: in function <spec/plugins/correlation-id/access_spec.lua:29>

[----------] 0 tests from spec/plugins/correlation-id/access_spec.lua (1014.09 ms total)

ahmadnassri · 2016-03-22T20:29:58Z

Fair enough! But mockbin is a debugging/development tool :) I think we can all agree on why this should not be the default behavior for our use case.

agreed, and mockbin should also not override such headers if they exist: Kong/insomnia-mockbin#45

opyate · 2016-03-22T20:43:56Z

Silly me: json.decode(response).headers aren't the real headers, but a mockbin -specific thing.

I'll change the spec to test for actual headers, but perhaps @thibaultcha can look at the Cassandra-related problem that's blocking my testing since the rebase (yes, I have the latest next).

thibaultcha · 2016-03-22T20:44:43Z

@opyate Seems your migrations did not run correctly for some reason.

- also revert busted:match, since it's not for regex

opyate · 2016-03-23T07:55:04Z

@thibaultcha I've committed the following:

changed tests to test actual headers, not mockbin-response-headers
added extra test for the case where downstream doesn't echo the correlation header
reverted the busted:match stuff - I don't think it does patterns ;)

Also, following the keyspace mess from yesterday, I think the Makefile clean section can benefit from something like this:

TMPDK=$(mktemp -t kongXXXXXX) && echo -e "drop keyspace kong_tests;\ndrop keyspace kong_development;" > $TMPDK && cassandra-cli -f $TMPDK && rm $TMPDK

coveralls · 2016-03-23T07:57:02Z

Coverage remained the same at 59.467% when pulling 4e12036 on opyate:correlation-id into efff148 on Mashape:next.

thibaultcha · 2016-03-23T08:31:55Z

Yes it does patterns, I have used it numerous times. See the luassert code base if doubts persist.

The make clean command is intended to be run without the datastores running. Anyways, such "messes" as you say are rare and would only occur during development, as migrations aren't always fixed until releasing. That is fine.

On Mar 23, 2016, at 12:55 AM, Juan M Uys [email protected] wrote:

@thibaultcha I've committed the following:

changed tests to test actual headers, not mockbin-response-headers
added extra test for the case where downstream doesn't echo the correlation header
reverted the busted:match stuff - I don't think it does patterns ;)
Also, following the keyspace mess from yesterday, I think the Makefile clean section can benefit from something like this:

TMPDK=$(mktemp -t kongXXXXXX) && echo -e "drop keyspace kong_tests;\ndrop keyspace kong_development;" > $TMPDK && cassandra-cli -f $TMPDK && rm $TMPDK
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

thibaultcha · 2016-03-23T08:37:24Z

Heh, it seems clean is already using the migrations. Mixing with other projects... But the issue here is that one is free to have as many keyspaces as desired (even for testing, I often use several ones) and thus the command can never know which ones are in an inconsistent state. Plus deleting other keyspaces, even developement is very unsafe and might not always be desired. Only the tests keyspace should be deleted without too much worrying.
In fact I'm not even such a fan of having clean doing this job. Will probably change to reflect the behavior of said projects.

opyate · 2016-03-23T16:32:47Z

See the luassert code base if doubts persist.

@thibaultcha Found it - apologies for doubting you :)

This should be it!

coveralls · 2016-03-23T16:55:03Z

Coverage remained the same at 59.467% when pulling 52756e9 on opyate:correlation-id into efff148 on Mashape:next.

thibaultcha · 2016-03-25T21:18:25Z

Hey,

I agree that this looks ready! A couple last things:

Would you please write some documentation for your plugin at https://github.com/Mashape/getkong.org? Thanks!
Could you squash your commits into 1, and maybe rebase on next? If you don't want to go through the troubles, I'll do it when applying the patch, no worries.
And finally: is everybody satisfied with this plugin name? I am asking since as of today, it is very difficult to change the name of an existing (shipped) plugin, so let's be sure it is clear to everyone. I agree that the current name makes perfect sense, but maybe users would search for such a feature under a different name, such as "request-id"? Just bringing the point to make sure correlation-id makes sense for everyone :)

thibaultcha · 2016-03-25T21:18:57Z

spec/plugins/correlation-id/access_spec.lua

+
+    assert.are_not_equals(correlation_id1, correlation_id2)
+
+    -- TODO kong_TEST.yml's worker_processes has to be 1 for the below to work.


It will be in the future :)

Tieske · 2016-03-25T23:32:58Z

how about "request-tracker"?

sonicaghi · 2016-03-26T02:53:14Z

Requests Trace

jl91 · 2016-03-31T20:11:56Z

a similar problem here #1118

subnetmarco · 2016-04-06T05:58:13Z

What is the status of this PR?

opyate · 2016-04-06T08:23:31Z

What is the status of this PR?

As the commiter, here's my opinion:

there was mention of a plugin rename, but I've only seen 3 or so disparate suggestions, so kept it as is
@thibaultcha will squash the commits when the patch gets incorporated into the mainline
documentation for the plugin is at documentaton for correlation-id plugin docs.konghq.com#211

codebien · 2016-04-06T10:25:11Z

IMHO "Web people" know this feature as Request-ID.

opyate · 2016-04-06T11:14:14Z

Tried to rebase to latest next, and now

stat: cannot stat `kong/api/route_helpers.lua': No such file or directory

Error: Build error: Failed installing kong/api/route_helpers.lua in /usr/local/lib/luarocks/rocks/kong/0.7.0-0/lua/kong/api/route_helpers.lua
make: *** [install] Error 1

thibaultcha · 2016-04-11T20:12:44Z

Hey @opyate, no worries, I have manually merged this to git next!

Squashed your patch
Slightly tweaked the tests to check for headers received by upstream as well
Added the new plugin to the unreleased changelogs.

Thanks a lot for contributing on this!

thibaultcha · 2016-04-11T20:13:52Z

PS: correlation-id will do it for a name. It is, after all, the technical term for such a feature :)
PPS: we'll try to get this included in the 0.8 release

opyate · 2016-04-12T09:47:18Z

Thanks a lot for contributing on this!

Thanks for all the assistance!

opyate mentioned this pull request Mar 22, 2016

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

Closed

thibaultcha reviewed Mar 22, 2016
View reviewed changes

thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Mar 22, 2016

opyate added 3 commits March 23, 2016 07:13

correlation-id plugin, fixes Kong#1086

5faae63

use more idiomatic spec matcher

e90ee34

make 'echo downstream' configurable and false default

4e12036

- also revert busted:match, since it's not for regex

opyate force-pushed the correlation-id branch from d612dc0 to 4e12036 Compare March 23, 2016 07:44

use luassert regex matcher

52756e9

thibaultcha reviewed Mar 25, 2016
View reviewed changes

opyate mentioned this pull request Mar 30, 2016

documentaton for correlation-id plugin Kong/docs.konghq.com#211

Closed

thibaultcha closed this Apr 11, 2016

thibaultcha removed the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Apr 11, 2016


		assert.are_not_equals(correlation_id1, correlation_id2)

		-- TODO kong_TEST.yml's worker_processes has to be 1 for the below to work.

correlation-id plugin, fixes #1086 #1094

correlation-id plugin, fixes #1086 #1094

Conversation

opyate commented Mar 22, 2016

thibaultcha Mar 22, 2016

Choose a reason for hiding this comment

opyate Mar 22, 2016

Choose a reason for hiding this comment

thibaultcha Mar 22, 2016

Choose a reason for hiding this comment

thibaultcha Mar 22, 2016

Choose a reason for hiding this comment

thibaultcha commented Mar 22, 2016

coveralls commented Mar 22, 2016

thibaultcha commented Mar 22, 2016

opyate commented Mar 22, 2016

thibaultcha commented Mar 22, 2016

ahmadnassri commented Mar 22, 2016

opyate commented Mar 22, 2016

thibaultcha commented Mar 22, 2016

opyate commented Mar 22, 2016

coveralls commented Mar 22, 2016

thibaultcha commented Mar 22, 2016

opyate commented Mar 22, 2016

ahmadnassri commented Mar 22, 2016

opyate commented Mar 22, 2016

thibaultcha commented Mar 22, 2016

opyate commented Mar 23, 2016

coveralls commented Mar 23, 2016

thibaultcha commented Mar 23, 2016

thibaultcha commented Mar 23, 2016

opyate commented Mar 23, 2016

coveralls commented Mar 23, 2016

thibaultcha commented Mar 25, 2016

thibaultcha Mar 25, 2016

Choose a reason for hiding this comment

Tieske commented Mar 25, 2016

sonicaghi commented Mar 26, 2016

jl91 commented Mar 31, 2016

subnetmarco commented Apr 6, 2016

opyate commented Apr 6, 2016

codebien commented Apr 6, 2016

opyate commented Apr 6, 2016

thibaultcha commented Apr 11, 2016

thibaultcha commented Apr 11, 2016

opyate commented Apr 12, 2016