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/groundwork for zipkin #3842

Closed
wants to merge 3 commits into from
Closed

Conversation

kikito
Copy link
Member

@kikito kikito commented Oct 10, 2018

This PR adds a couple features to the PDK:

  • kong.request.get_start_time(), which wraps ngx.req.start_time()
  • kong.request.get_raw_path_and_query()

It also fix a (possible?) problem when ngx.request_uri is nil.

It is needed for the zipkin update: Kong/kong-plugin-zipkin#24

@kikito kikito changed the base branch from master to next October 10, 2018 09:21
@kikito kikito force-pushed the feat/pdk-extensions-for-zipkin branch from 82666cf to f9137bc Compare October 10, 2018 15:13
@kikito kikito changed the title Feat/pdk extensions for zipkin Feat/groundwork for zipkin Oct 16, 2018
@kikito kikito force-pushed the feat/pdk-extensions-for-zipkin branch from ebbc696 to 4830d28 Compare October 16, 2018 16:39
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@kikito kikito force-pushed the feat/pdk-extensions-for-zipkin branch from 359f24b to 2241fbd Compare October 16, 2018 18:19
-- -- Given a request to https://example.com:1234/v1/movies?movie=foo
--
-- kong.request.get_path_and_querystring() -- "/v1/movies?movie=foo"
function _REQUEST.get_path_and_querystring()
Copy link
Member

Choose a reason for hiding this comment

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

I left comment about the name here:
#3859 (review)

Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

deleted

@bungle
Copy link
Member

bungle commented Oct 17, 2018

I am not sure if ngx.var.request_uri can ever be nil, at least with http module. How could it be?

Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

deleted

@kikito kikito force-pushed the feat/pdk-extensions-for-zipkin branch from 9f2a159 to 5aa253b Compare October 17, 2018 14:02
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

LGTM

When the path + querystring is needed, this prevents having to manually check wether the querystring is present and concatenating the path + "?" + the query string every time.

Needed for the zipkin plugin.
This came out while reviewing the zipkin plugin. I was not able to
reproduce the problem in our tests. It certainly does not hurt to have
the extra `or ""` anyway
@kikito kikito force-pushed the feat/pdk-extensions-for-zipkin branch from 5aa253b to 5ef4583 Compare October 17, 2018 21:07
thibaultcha pushed a commit that referenced this pull request Oct 17, 2018
When the path + querystring is needed, this prevents having to manually
check wether the querystring is present and concatenating the path + "?"
+ the query string every time.

From #3842

Signed-off-by: Thibault Charbonnier <[email protected]>
thibaultcha pushed a commit that referenced this pull request Oct 17, 2018
This came out while reviewing the zipkin plugin. I was not able to
reproduce the problem in our tests. It certainly does not hurt to have
the extra `or ""` anyway.

From #3842

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

Merged to next with adjustments; except for the timing API commit

@thibaultcha thibaultcha deleted the feat/pdk-extensions-for-zipkin branch October 17, 2018 22:49
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.

4 participants