Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

feat(handler) static tags #84

Merged
merged 1 commit into from
Jun 10, 2020
Merged

feat(handler) static tags #84

merged 1 commit into from
Jun 10, 2020

Conversation

kikito
Copy link
Member

@kikito kikito commented May 25, 2020

Allows setting tags on the request span via configuration.

Implements #3

Copy link

@seh seh left a comment

Choose a reason for hiding this comment

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

I'm excited to see this patch.

Thinking of the scopes at which one can define these tags, is it possible to associate a plugin configuration with a particular Kubernetes Ingress object, or all the Ingress objects in a given namespace, as opposed to defining them globally?

kong/plugins/zipkin/handler.lua Show resolved Hide resolved
kong/plugins/zipkin/schema.lua Show resolved Hide resolved
kong/plugins/zipkin/schema.lua Show resolved Hide resolved
@@ -205,6 +213,14 @@ elseif subsystem == "stream" then

request_span:set_tag("lc", "kong")

local static_tags = conf.static_tags
if type(static_tags) == "table" then
for i = 1, #static_tags do
Copy link

Choose a reason for hiding this comment

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

How do you choose between this technique and using ipairs?

Copy link
Member

Choose a reason for hiding this comment

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

This one I'm not sure but if you do get confused between pairs and ipairs, we use ipairs in the hot code paths to increase the probability that it is compiled by the JIT: http://wiki.luajit.org/NYI

Copy link
Member Author

Choose a reason for hiding this comment

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

Performance wise, both alternatives have similar speed (both get JITted) in LuaJIT. I think ipairs is more readable, so if you are sure your code is going to get executed exclusively in LuaJIT, you should use ipairs.

However, if there's a chance your code is not going to be executed in LuaJIT, the performance of ipairs is way worse than the numeric alternative in Vanilla Lua.

Since I maintain several non-LuaJIT-specific open source Lua libraries I always use the numeric for "on automatic" - I almost never use ipairs. I had internalized this so much that I had to think and reevaluate my thought process :)

@@ -15,6 +40,7 @@ return {
{ traceid_byte_count = { type = "integer", required = true, default = 16, one_of = { 8, 16 } } },
{ header_type = { type = "string", required = true, default = "preserve",
one_of = { "preserve", "b3", "b3-single", "w3c" } } },
{ static_tags = { type = "array", elements = static_tag } }
Copy link

Choose a reason for hiding this comment

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

I doesn't look like there's a way to enforce in the schema that the static_tag values' names are unique. The last one wins if there are multiple tags with the same name. Should we validate that the names are unique?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now included (code and tests)

@hbagdi
Copy link
Member

hbagdi commented May 27, 2020

Thinking of the scopes at which one can define these tags, is it possible to associate a plugin configuration with a particular Kubernetes Ingress object, or all the Ingress objects in a given namespace, as opposed to defining them globally?

Based on reading the existing code, it seems this plugin can be enabled on any service or route in Kong.
@kikito Can you confirm?

That means, in the k8s world, you can associate a specific plugin configuration to:

  • A Ingress object
  • A Service
  • Or globally

We currently don't support running plugins for all ingress resource on a namespace. That's a limitation/feature of the controller and not this specific plugin.

@kikito
Copy link
Member Author

kikito commented Jun 8, 2020

@hbagdi I confirm the plugin can be enabled on a Service or Route.

Allows setting tags on the request span via configuration.

Implements #3
@kikito kikito merged commit 1ffbef0 into master Jun 10, 2020
@kikito kikito deleted the feat/static-tags branch June 10, 2020 14:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants