Introduce Span#set_tags

[DocX]

In many places it is reasonable to set span tags from a hash. Currently the only way was to iterate thru the hash and call set_tag. set_tags implements this directly in Span so user code does not need to repeat it everytime tags are set from a hash.

[tjwp]

In our user code support for setting tags from hashes we recurse to flatten nested hashes along these lines:

def set_tags(tags, prefix = nil)
  tags.each do |k, v|
    key = prefix.nil? ? k : "#{prefix}.#{k}"
    v.is_a?(Hash) ? set_tags(v, key) : set_tag(key, v)
  end
end

[delner]

@tjwp Can you explain why you would send a nested hash to this function? I can see how the nested representation makes sense, its just not clear to me how one would end up with a nested set of tags in the first place.

[delner]

@DocX Looks like a pretty straightforward addition to the API; I like that its kept simple in this way, and that it'll make syntax a little easier for users. Nice work adding tests for this too!

Looks like Excon tests are broken, but I suspect that has less to do with your changes here, and more to do with some new release. We'll need you to rebase once the issue is fixed and merged, but otherwise I think this should be ready.

[tjwp]

@tjwp Can you explain why you would send a nested hash to this function? I can see how the nested representation makes sense, its just not clear to me how one would end up with a nested set of tags in the first place.

We have existing instrumentation in our code that calls an abstraction layer. Some previous products we've used accept a nested hash for attributes so that is what our abstraction accepts.

The code that I showed is roughly how we handle that nested hash today for Datadog APM.

More generally, sometimes in our code we already have data as a nested hash that we want to add as context for a span in a trace, so it is more natural to accept that nested hash as an argument than to require the caller to do additional flattening or formatting in order to send it to APM.

Our approach also gives us good fidelity because if we flatten the hash to set tags with dot-separated names then the API displays them like a nested "hash". To me it makes sense if you want to see a nested hash/object in the UI, then you specify it as a nested hash in the code

[DocX]

wonder if the unless tags.emoty is even necessary (I mean it is not, but from readability point of view?)

[delner]

Doesn't look necessary strictly speaking. The question is what's less expensive; checking empty? or entering another function, then each on an empty hash?

It's probably less expensive doing empty? if I had to guess; you could benchmark it if you were so inclined, but I'd be fine with it as it is.

[delner]

@tjwp I see.

To me it makes sense if you want to see a nested hash/object in the UI, then you specify it as a nested hash in the code.

I think this is the best argument that could be made, but still how the nesting is presented is more a feature of the UI and less of building an appropriate span data model. It assumes . will be used as a delimiter to always drive a UI feature, and I think that limits flexibility in our ability to present data differently.

In the general sense, fidelity to the trace data model and performance are among the two highest priorities for this library, and to that end I'd like the behavior to remain as simple and as lean as possible. My sense is support for nesting is an abstraction that not all or even most users will leverage, and it will only serve to make the default path slower. If its necessary for some compatibility layer, then that's probably where it should exist.

[tjwp]

@delner No problem. Since the PR description noted that this was likely already happening in "user" code implementations, I thought I'd share what we're doing in ours. The implementation here means that we will continue to use our own abstraction layer and call set_tag directly instead of using this new method, but that's okay.

[DocX]

@delner Hi, I've rebased and the tests are fixed. Do you think it would be now ok to merge it?

[marcotc]

Thank you, @DocX!