Add the `:record_on_server_receive` configuration option #103

jfeltesse-mdsol · 2017-04-21T08:05:22Z

This adds a new configuration option to force the sending of path info on server receive.
Usually it is not needed because it's provided by the client but we have a use case here that requires that data to be sent (in a nutshell some service processing zipkin data to compute metrics).

@jcarres-mdsol @adriancole

codefromthecrypt · 2017-04-21T09:36:03Z

lib/zipkin-tracer/config.rb

      if @sampled_as_boolean
        @logger && @logger.warn("Using a boolean in the Sampled header is deprecated. Consider setting sampled_as_boolean to false")
      end
+      # Record the path info on server receive, even if the zipkin headers were present in the incoming request?


I've thought about this a little elsewhere and kindof like the keys approach, where you specify the keys you would like to have added to the span. I'm not sure if this config is shared between client and server or not. If it isn't you could imagine a set that includes "http.uri" etc.

You mean the configuration file would have a list of keys to be added on each span for both the client and the server?

codefromthecrypt · 2017-04-21T09:45:12Z

You mean the configuration file would have a list of keys to be added on each span for both the client and the server?

yep. this is particularly neat with http.url vs http.uri, but also when people add new keys like "requestId" as then you can declaratively decide whether to add it even if the code is always there

jfeltesse-mdsol · 2017-04-24T07:19:09Z

@adriancole talking with @jcarres-mdsol offline we came to the conclusion that I'm going to change that configuration option to something more generic that would allow you to specify keys that are recognized by the client/server, such as http.path, in CSV style.
Note that right now it's only that one which is "duplicated", the client doesn't send http.uri for instance.
I'll amend this PR and comment back.

codefromthecrypt · 2017-04-24T07:26:40Z

sounds great. I'm excited to see where this goes. we might use similar approach in other tracers.

jfeltesse-mdsol · 2017-04-24T09:39:48Z

changed the option to :record_on_server_receive which accepts CSV style tag names, whitelisting them though. It only supports http.path for now, not sure what would make sense to add...

jcarres-mdsol · 2017-04-25T04:46:18Z

lib/zipkin-tracer/config.rb

      end
+      # Record the given tags on server receive, even if the zipkin headers were present in the incoming request?
+      tags = config[:record_on_server_receive]
+      @record_on_server_receive = present?(tags) ? parse_record_on_server_receive(tags) : nil


I would do {} instead of nil and later you do not need to check for nil just do .each on the hash

the reason I went this way is that the standard case is the user doesn't have this option set and the service is called by another zipkin-enabled one. In this 99% of the time case, we don't need to do anything at all, e.g.

zipkin_env.called_with_zipkin_headers? is true

@config.record_on_server_receive is falsy
=> tags var is nil
=> trace_request_information doesn't do anything

If I go the default empty hash route here I'll either have to check for nil and empty or I'll have to create a new hash object every time just so that .each doesn't blow.

The empty hash would be made only here in config and then stay empty, then you do not need to check for it, maybe you'll need to call merge on it and each.

On the other hand nil brings no meaning, given that something blows up and it is a nil, it is difficult to know the intention of that data

something like this? c41332a

jcarres-mdsol · 2017-04-25T04:47:03Z

lib/zipkin-tracer/rack/zipkin-tracer.rb

  # This middleware reads Zipkin headers from the request and sets/creates a Trace.id usable by the rest of the app
  # It will also send the trace to the Zipkin service using one of the methods configured.
  class RackHandler
+    # the following constants are defined only from rack 1.6


could we depend on rack > 1.6 ?

Not really if we want to keep a reasonable backwards-compatibility.
For instance actionpack 4.1.x depends on rack ~> 1.5.2 (https://rubygems.org/gems/actionpack/versions/4.1.16)

codefromthecrypt · 2017-04-25T06:26:49Z

lib/zipkin-tracer/config.rb

      sampled_as_boolean: true
    }

+    def parse_record_on_server_receive(tag_names)


I don't know what's idiomatic, but seems like it would be easier to see this if this function were in the rack class (ex tag filter was passed to it vs moving the logic here). For example, it would make it easier to see if you change "parse_record_on_server_receive" to "parse_tags" and no meaning is loss. I was looking to see if any tags were added on server send and needed to scroll to the rack handler file to find that out..

Not sure I get it, here I'm parsing the value passed in the config once and for all, just like the other config options. I don't think that parsing whatever was passed at config time belongs in the rack class since it has nothing to do with middleware stuff.

codefromthecrypt · 2017-04-25T08:12:09Z

lib/zipkin-tracer/rack/zipkin-tracer.rb

      tags = DEFAULT_SERVER_RECV_TAGS unless zipkin_env.called_with_zipkin_headers?
-      # if the user specified tags to record on server recv, use these no matter what
-      tags = @config.record_on_server_receive if @config.record_on_server_receive
+      # if the user specified tags to record on server receive, use these no matter what


nit spelling receive

? Am I missing something? Any spelling issue with the following?
"if the user specified tags to record on server receive, use these no matter what"

weird.. I thought I saw "recieve" here! Ignore me .. I'm not helping I think :P

codefromthecrypt · 2017-04-25T08:13:02Z

sorry I misunderstood. thanks for clearing!

jcarres-mdsol · 2017-04-25T08:28:38Z

lib/zipkin-tracer/rack/zipkin-tracer.rb

-      # if the user specified tags to record on server recv, use these no matter what
-      tags = @config.record_on_server_receive if @config.record_on_server_receive
+      # if the user specified tags to record on server receive, use these no matter what
+      tags = @config.record_on_server_receive unless @config.record_on_server_receive.empty?


why do this? just let the empty hash reach trace_request_information

true, that was in line with the old tags can be nil code but it doesn't make sense anymore

ah, nope, can't do this because of the case where we'd been called by a non zipkin enabled service, tags would be using the DEFAULT_SERVER_RECV_TAGS. Getting rid of the check here would overwrite this and make it empty...
And I can't use that DEFAULT_SERVER_RECV_TAGS as the config's default altogether because we still need it to be empty in the 99% case where we're called with zipkin headers and we don't want the http.path to be recorded too.
In a nutshell, we need the empty default config while having an alternative default for the case of no zipkin headers in the request.
Makes sense?

jcarres-mdsol · 2017-04-26T03:10:48Z

lib/zipkin-tracer/rack/zipkin-tracer.rb

-      # if called by a service, the caller already added the information
-      trace_request_information(span, zipkin_env.env) unless zipkin_env.called_with_zipkin_headers?
+      # if the request comes from a non zipkin-enabled source record the default tags
+      tags = DEFAULT_SERVER_RECV_TAGS unless zipkin_env.called_with_zipkin_headers?


maybe do these things inside trace_request_information ?

jcarres-mdsol · 2017-04-26T03:15:07Z

lib/zipkin-tracer/rack/zipkin-tracer.rb

+    def trace_request_information(span, env, tags)
+      # tags is nil if we've been called by a zipkin-enabled service and the user hasn't
+      # specified tags to record on server receive.
+      return if tags.nil?


Maybe...

tags = if !@config.record_on_server_receive.empty? @config.record_on_server_receive else if !zipkin_env.called_with_zipkin_headers? DEFAULT_SERVER_RECV_TAGS else {} end

updated following your recommendation but left the return if tags.nil? check so as not to needlessly create empty hash objects and call .each on them in what is going to be the 99% use case.

codefromthecrypt reviewed Apr 21, 2017

View reviewed changes

Add the :record_on_server_receive configuration option

94fa8e4

jfeltesse-mdsol changed the title ~~Add the :always_record_path configuration option~~ Add the :record_on_server_receive configuration option Apr 24, 2017

jcarres-mdsol reviewed Apr 25, 2017

View reviewed changes

codefromthecrypt reviewed Apr 25, 2017

View reviewed changes

make config.record_on_server_receive an empty hash by default

c41332a

codefromthecrypt reviewed Apr 25, 2017

View reviewed changes

jcarres-mdsol reviewed Apr 25, 2017

View reviewed changes

jcarres-mdsol reviewed Apr 26, 2017

View reviewed changes

refactor ZipkinTracer::RackHandler#trace_request_information

53a04c1

jcarres-mdsol merged commit 5aa6afd into openzipkin:master Apr 26, 2017

jfeltesse-mdsol deleted the feature/always_record_path_option branch April 26, 2017 09:13

Add the :record_on_server_receive configuration option #103

Add the :record_on_server_receive configuration option #103

Uh oh!

Conversation

jfeltesse-mdsol commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Apr 21, 2017 via email

Uh oh!

jfeltesse-mdsol commented Apr 24, 2017

Uh oh!

codefromthecrypt commented Apr 24, 2017 via email

Uh oh!

jfeltesse-mdsol commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Apr 25, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add the `:record_on_server_receive` configuration option #103

Add the `:record_on_server_receive` configuration option #103