Fix redis integration not to create an invalid span in case of empty pipeline #757
Conversation
delner
added
bug
|
The failed build on Ruby 2.1 is unrelated. This is notorious |
|
No problem, this happens time to time. I kicked off another build for you, seemed like it passed. I'll review and let you know! |
lib/ddtrace/contrib/redis/patcher.rb
Outdated
| span.set_metric Datadog::Contrib::Redis::Ext::METRIC_PIPELINE_LEN, commands.length | ||
| commands = args[0].commands.map { |c| Datadog::Contrib::Redis::Quantize.format_command_args(c) } | ||
|
|
||
| if commands.any? |
There was a problem hiding this comment.
So this is interesting because it raises the question that if there are no commands, but the pipeline still runs, do we want to instrument it? (There might be a case for either one. I can imagine you might want to see that the Redis pipeline ran, even if it didn't do anything.)
If no, then I think this change is fine. If yes, then maybe we should give the resource name something else that isn't blank (e.g. null, (none), NOOP etc.).
Thoughts?
There was a problem hiding this comment.
@delner Agree. I thought about it before and it seemed like a piece of useless information but at the moment it makes more sense. Understanding that your pipeline doesn't make any commands is a great insight into a transaction part.
I'll change the implementation.
|
@delner let's keep this PR at the top of our list, so we can move forward with @sponomarev work! Thanks for this really helpful contribution! |
|
@marcotc this seems like a trivial rebase away from a nice win, what do you think? |
|
Thank you so much for the endurance in getting this PR merged, @sponomarev! 🙇 Sorry it took us so long to follow up before. It will be shipped in the next release, 1.3.0. |
At the moment, if
Redis#pipelinedblock doesn't invoke any Redis commands, the integration creates an invalid span with empty lineResourceattribute. As a result, an agent drops that span and the whole trace.Empty pipeline case is pretty legit for dynamic keys gathering. For instance, in Sidekick.
This PR solves the problem and creates a
(none)span for an empy pipeline.