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

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

Conversation

ykitamura-mdsol
Copy link
Contributor

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 #130

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

@@ -26,8 +26,6 @@ def initialize(app, config_hash)
@sample_rate = config[:sample_rate] || DEFAULTS[:sample_rate]
# A block of code which can be called to do extra annotations of traces
@annotate_plugin = config[:annotate_plugin] # call for trace annotation
@filter_plugin = config[:filter_plugin] # skip tracing if returns false
@whitelist_plugin = config[:whitelist_plugin] # force sampling if returns true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to set twice (see below) 😎

@jfeltesse-mdsol
Copy link
Contributor

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

Otherwise LGTM.

@jcarres-mdsol
Copy link
Collaborator

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
Copy link
Collaborator

jcarres-mdsol commented Mar 26, 2019

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
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor Author

closing in favor of #143

@ykitamura-mdsol ykitamura-mdsol deleted the feature/additional_paths branch April 5, 2019 15:07
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