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

Wire up zipkin stats receivers #4

Closed
wants to merge 2 commits into from
Closed

Wire up zipkin stats receivers #4

wants to merge 2 commits into from

Conversation

klingerf
Copy link
Contributor

Use the stats receiver provided by the stack when initializing the http and kafka tracers. Fixes #3.

@klingerf klingerf self-assigned this Feb 13, 2017
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

you should double check how these stats actually get exported. I'd expect the stats scope to be ("telemeters", configId), but we may need to fix that on the linkerd site if that's not done properly today.

KafkaZipkinTracer.create(config, NullStatsReceiver)
def mk(params: Stack.Params): KafkaTelemeter = {
val param.Stats(stats) = params[param.Stats]
val tracer = KafkaZipkinTracer.create(config, stats.scope("zipkin.kafka"))
Copy link
Member

Choose a reason for hiding this comment

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

i know we're nowhere near consistent about this, but let's call this "io.zipkin.kafka" to match the plugin name or even configID / topic if we think it's reasonable to run multiple telemeters to different topics

HttpZipkinTracer.create(config, NullStatsReceiver)
def mk(params: Stack.Params): HttpTelemeter = {
val param.Stats(stats) = params[param.Stats]
val tracer = HttpZipkinTracer.create(config, stats.scope("zipkin.http"))
Copy link
Member

Choose a reason for hiding this comment

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

similarly

@klingerf
Copy link
Contributor Author

Ok, I updated the stats prefixes, and that actually helped to determine that this branch is not working correctly. With the http tracer configured, here are all of the zipkin stats that show up in metrics.json: https://gist.github.com/klingerf/b63b33ad68dfe65278091abcb3f13e8b.

The only new stat in that payload that appears as a result of this change is:

io.zipkin.http/record/unhandled/scala.collection.immutable.Set$Set1

Which is clearly incorrect. The rest of the stats in the payload appear to be happening automatically, and are not tied to the stats receiver that's used to initialize the tracers from this repo. As far as I understand it, this is where all of these stats appear to be coming from:

  • The clnt/zipkin-http/* stats are exported by the configured zipkin-finagle http tracer, which is being used to send traces, and has non-zero stats.

  • The clnt/zipkin-tracer/* stats are exported by finagle's default zipkin scribe tracer, which is autoloaded but unused, so all of these stats are 0.

  • The zipkin.http/* stats are exported by the autoloaded zipkin-finagle http tracer, which is also unused, so all of these stats are 0.

  • The zipkin.kafka/* stats are exported by the autoloaded zipkin-finagle kafka tracer, which is also unused, so all of these stats are 0.

It would be great to disable autoloading for all of these tracers that are unused. And additionally, it would be good to fix the stats receiver issue in zipkin-finagle that's causing the malformed stats. In the meantime though, I don't think it makes sense to proceed with this change since we're already getting stats from the zipkin-finagle http tracer, and the scoping on those stats is not configurable from this repo, as far as I can tell.

@adleong
Copy link
Member

adleong commented May 1, 2018

@klingerf should we close this PR then?

@klingerf
Copy link
Contributor Author

@adleong Yeah, I think it makes sense to close this. Thanks for checking.

@klingerf klingerf closed this May 10, 2018
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.

3 participants