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

feat(zipkin) override unknown-service-name #43

Closed
wants to merge 3 commits into from
Closed

feat(zipkin) override unknown-service-name #43

wants to merge 3 commits into from

Conversation

jeremyjpj0916
Copy link
Contributor

@jeremyjpj0916 jeremyjpj0916 commented Jul 24, 2019

Optional configurable field to enable setting a defaulted service name in the zipkin spans to replace the unknown-service-name existing behavior,

Fixes: #39
Fixes: #23 (I think if I am reading it correctly)

I defaulted the string to nil as to not change any behavior of the plugin by default. Could easily set default to "Kong" if that is preferred but that would change default behavior so I didn't commit it like that.

@devinsba
Copy link

Sorry for the drive-by but as a zipkin contributor I watch a lot of repos to see how people are using it. I would disagree with this change. I don't think a proxy/lb/api-gateway should set localServiceName to the name of the service it is proxying to, I would expect localServiceName to be something like internal-apps-api-gateway or something to that affect.

Ultimately what I'm saying is I don't think this is the correct way to fix #39 within the zipkin model, and I agree with the original poster of #23 on the way forward being a constant for the entire instance

cc @adriancole

@james-callahan
Copy link
Contributor

Sorry for the drive-by but as a zipkin contributor I watch a lot of repos to see how people are using it. I would disagree with this change. I don't think a proxy/lb/api-gateway should set localServiceName to the name of the service it is proxying to, I would expect localServiceName to be something like internal-apps-api-gateway or something to that affect.

That's not what this PR does.
Rather, it allows the user to configure, on a per zipkin plugin basis, what the service should be called.
I imagine the common configuration would be to configure the plugin globally, which gives the effect you mention.

@jeremyjpj0916
Copy link
Contributor Author

jeremyjpj0916 commented Jul 24, 2019

Yeah I specifically avoided setting it to the service name of where Kong is proxying to. We get that in the kong.proxy span logged already. This is a static config as @james-callahan described above. We currently run this plugin globally too with a fork with this change essentially because we wanted to rid ourselves of the unknown-service-name behavior the plugin currently displays on other spans outside the kong.proxy span:

Example Image

In our case that tag gets changed to StarGate (which is the internal pet name we gave our gateway stack with Kong at its core). I could see people naming it "Kong" or just about any static they want to associate with a specific zipkin plugin instance executing.

@devinsba
Copy link

Ok, then I was confused by the peer portion of the variable name looks like what I would expect then

@jeremyjpj0916
Copy link
Contributor Author

jeremyjpj0916 commented Jul 24, 2019

Yeah, I am open to alternative variable names, wasn't sure whats best. Maybe default_service_name or something.

Edit: Adjusted it to that as well in PR.

@jeremyjpj0916
Copy link
Contributor Author

jeremyjpj0916 commented Jul 24, 2019

Oof, I done goofed in my commit squashing/rebasing now I think 😆

Edit - I am a SCRUBBBBBBB, faster to just fork and PR this again rather than fix w/e damage I have done to my local lol. Hang tight.

@jeremyjpj0916
Copy link
Contributor Author

Closing this one, continuing over here: #44

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants