openzipkin / zipkin-ruby

zipkin-tracer ruby gem
Apache License 2.0
100 stars 38 forks source link

Add the ':additional_paths' configuration option to enable tracing on non-Rails-routable paths #142

Closed ykitamura-mdsol closed 5 years ago

ykitamura-mdsol commented 5 years ago

Add the :additional_paths configuration option to enable tracing on non-Rails-routable paths, such as in apps using Roda, Sinatra.

This is another approach to solve the issue in https://github.com/openzipkin/zipkin-ruby/pull/130

@cabbott @jcarres-mdsol @jfeltesse-mdsol @ssteeg-mdsol @piao-mdsol @adriancole

jfeltesse-mdsol commented 5 years ago

The naming might be too broad to explain what this does but I can't find a better alternative.

Otherwise LGTM.

jcarres-mdsol commented 5 years ago

It is not bad, I'm concerned if some day we want to base this decision on other thing other than paths, like headers or something, then we need to change this somehow...

jcarres-mdsol commented 5 years ago

I've read the related code, and it is a mess. Probably my fault.

There are different places where we do checks on sampling. We should consolidate.

The routable? information from the rack middleware should be moved here https://github.com/openzipkin/zipkin-ruby/blob/master/lib/zipkin-tracer/rack/zipkin_env.rb

And now we have all the stuff in there.

Then here this would be the logic: trace.sampled = parent.sampled && !filter_plugin() && (whitelist_plugin() || routable? )

This is set to the trace once here and then we can delete from faraday and excon the check on if something is routable.

We thus do not need to add a new configuration key. We need to fix how the whitelist plugin is used.

To achieve the original intent, someone will pass something like ->(env) { env["PATH_INFO"] ~ whitelisted_regex }

to the whitelist plugin

ykitamura-mdsol commented 5 years ago

@jcarres-mdsol whitelist_plugin is used for force sampling now but do you mean you are going to make a backward-incompatible change? And I don't see any routable checks in faraday or excon... Would you please make a PR for this?

jcarres-mdsol commented 5 years ago

True. I'd just move routable_requests? to zipkin_env and change https://github.com/openzipkin/zipkin-ruby/blob/master/lib/zipkin-tracer/rack/zipkin_env.rb#L67 to this new_sampled_header_value(force_sample? || current_trace_sampled? && !filtered? && routable_request?)

ykitamura-mdsol commented 5 years ago

closing in favor of #143