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

[OpenTracing] Span and SpanContext implementation #472

Merged
merged 8 commits into from
Jun 22, 2018
2 changes: 1 addition & 1 deletion lib/ddtrace/opentracer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module OpenTracer
module_function

def supported?
Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.0')
Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.1')
end

def load_opentracer
Expand Down
77 changes: 77 additions & 0 deletions lib/ddtrace/opentracer/span.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,83 @@
module Datadog
module OpenTracer
# OpenTracing adapter for Datadog::Span
class Span < ::OpenTracing::Span
attr_reader \
:datadog_span

def initialize(datadog_span:, span_context:)
@datadog_span = datadog_span
@span_context = span_context
end

# Set the name of the operation
#
# @param [String] name
def operation_name=(name)
datadog_span.name = name
end

# Span Context
#
# @return [SpanContext]
def context
@span_context
end

# Set a tag value on this span
# @param key [String] the key of the tag
# @param value [String, Numeric, Boolean] the value of the tag. If it's not
# a String, Numeric, or Boolean it will be encoded with to_s
def set_tag(key, value)
tap { datadog_span.set_tag(key, value) }
end

# Set a baggage item on the span
# @param key [String] the key of the baggage item
# @param value [String] the value of the baggage item
def set_baggage_item(key, value)
tap { context.baggage[key] = value }
Copy link
Member

Choose a reason for hiding this comment

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

Do we want the span to be in charge of manipulating the data within the baggage?

By having the span context take care of manipulating the baggage we can abstract away the implementation details of the baggage.

Although it is unlikely that the implementation of the baggage will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I don't think the span should be responsible for knowing details about baggage on the span context. That's why I'm a bit confused about this: if the span context manages the baggage, why is there a baggage function on the span at all?

We could implement baggage functions on the span context object that mirror this, and have this span delegate to those. I guess I'm hesitant to do that only because it seems like a modification of the OT spec. But since its all internal anyways, maybe it's okay.

@Kyle-Verhoog What are your thoughts: move this to SpanContext and delegate from Span, or keep as is?

Copy link
Member

Choose a reason for hiding this comment

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

I think the delegate makes sense. Also, I forgot to mention this earlier, but we gotta keep in mind the span context instance should be immutable[0]. So setting a key in the baggage for a span context should produce a new span context instance with the key which the span will use.

[0] https://github.com/opentracing/specification/blob/master/specification.md#spancontext

Copy link
Member

@Kyle-Verhoog Kyle-Verhoog Jun 21, 2018

Choose a reason for hiding this comment

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

I think what this could look like is spancontext.set_baggage_item returns a new span context instance with the new item added and span.set_baggage_item will update its span context reference with this new instance.

(This is a change I have to make in the python tracer still).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kyle-Verhoog Okay, I think you're right about the immutable part. I think the implementation might need to be a little different: how about we don't implement SpanContext#set_baggage_item and force the use of SpanContext#new constructor which accepts baggage? Then do as you said, and have the Span#set_baggage_item build a new SpanContext from an old one?

The more complicated this SpanContext management gets, the more it screams "factory pattern" to me. Especially if the tracer has to build these too.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me! The SpanContext#new should duplicate the baggage that it receives. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll go ahead and make this change then.

end

# Get a baggage item
# @param key [String] the key of the baggage item
# @return [String] value of the baggage item
def get_baggage_item(key)
context.baggage[key]
end

# @deprecated Use {#log_kv} instead.
# Reason: event is an optional standard log field defined in spec and not required. Also,
# method name {#log_kv} is more consistent with other language implementations such as Python and Go.
#
# Add a log entry to this span
# @param event [String] event name for the log
# @param timestamp [Time] time of the log
# @param fields [Hash] Additional information to log
def log(event: nil, timestamp: Time.now, **fields)
super # Log deprecation warning

# If the fields specify an error
if fields.key?(:'error.object')
datadog_span.set_error(fields[:'error.object'])
end
end

# Add a log entry to this span
# @param timestamp [Time] time of the log
# @param fields [Hash] Additional information to log
def log_kv(timestamp: Time.now, **fields)
Copy link
Member

Choose a reason for hiding this comment

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

What about the other error fields (stack, event, etc)? Or does error.object basically encapsulate all of these?

All standard log fields:
https://github.com/opentracing/specification/blob/master/semantic_conventions.md#log-fields-table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Datadog::Span#set_error only accepts an StandardError object, which contains the message, stack, and type. That function then pulls those parts out and tags the span with them appropriately.

# If the fields specify an error
if fields.key?(:'error.object')
datadog_span.set_error(fields[:'error.object'])
end
end

# Finish the {Span}
# @param end_time [Time] custom end time, if not now
def finish(end_time: Time.now)
datadog_span.finish(end_time)
end
end
end
end
5 changes: 5 additions & 0 deletions lib/ddtrace/opentracer/span_context.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
module Datadog
module OpenTracer
# OpenTracing adapter for SpanContext
class SpanContext < ::OpenTracing::SpanContext
def initialize(baggage: {})
super
@baggage = baggage
end
end
end
end
3 changes: 2 additions & 1 deletion spec/ddtrace/opentracer/span_context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@

subject(:span_context) { described_class.new }

it { is_expected.to have_attributes(baggage: nil) }
it { is_expected.to have_attributes(baggage: {}) }

describe '#initialize' do
context 'given baggage' do
subject(:span_context) { described_class.new(baggage: baggage) }
let(:baggage) { { account_id: '1234' } }
it { is_expected.to be_a_kind_of(described_class) }
it { expect(span_context.baggage).to be(baggage) }
end
end
end
Expand Down
41 changes: 31 additions & 10 deletions spec/ddtrace/opentracer/span_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,38 +7,52 @@
RSpec.describe Datadog::OpenTracer::Span do
include_context 'OpenTracing helpers'

subject(:span) { described_class.new }
subject(:span) { described_class.new(datadog_span: datadog_span, span_context: span_context) }
let(:datadog_span) { instance_double(Datadog::Span) }
let(:span_context) { instance_double(Datadog::OpenTracer::SpanContext) }

describe '#operation_name=' do
subject(:result) { span.operation_name = name }
let(:name) { 'execute_job' }

before(:each) { expect(datadog_span).to receive(:name=).with(name).and_return(name) }
it { expect(result).to eq(name) }
end

describe '#context' do
subject(:context) { span.context }
it { is_expected.to be(OpenTracing::SpanContext::NOOP_INSTANCE) }
it { is_expected.to be(span_context) }
end

describe '#set_tag' do
subject(:result) { span.set_tag(key, value) }
let(:key) { 'account_id' }
let(:value) { '1234' }
before(:each) { expect(datadog_span).to receive(:set_tag).with(key, value) }
it { is_expected.to be(span) }
end

describe '#set_baggage_item' do
subject(:result) { span.set_baggage_item(key, value) }
let(:key) { 'account_id' }
let(:value) { '1234' }
let(:baggage) { instance_double(Hash) }

before(:each) do
allow(span_context).to receive(:baggage).and_return(baggage)
expect(baggage).to receive(:[]=).with(key, value)
end

it { is_expected.to be(span) }
end

describe '#get_baggage_item' do
subject(:result) { span.get_baggage_item(key) }
let(:key) { 'account_id' }
it { is_expected.to be nil }
let(:value) { '1234' }
let(:baggage) { { key => value } }
before(:each) { allow(span_context).to receive(:baggage).and_return(baggage) }
it { is_expected.to be(value) }
end

describe '#log' do
Expand All @@ -47,23 +61,30 @@
let(:timestamp) { Time.now }
let(:fields) { { time_started: Time.now, account_id: '1234' } }

before(:each) do
# Expect a deprecation warning to be output.
it do
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it 'expects a deprecation warning to be output.' work here?

Copy link
Contributor Author

@delner delner Jun 21, 2018

Choose a reason for hiding this comment

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

You could do that, and that would be okay. What matters is that the test is easy to read and understand. The language as its written reads as "it expects log to output 'Span#log is deprecated.' to stderr." In my mind, I think this is an abundantly clear statement, that does not require clarification by naming the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to bikesched too much. I have mixed feelings about using nameless examples. Having explicit name to a example most often than not makes it more clearer than the code itself.

And while 1 line and relatively short nameless examples are fine and easy to read most of the time.
However examples like:

it do
 expect {x}.to change {z}
end

look like a exception from this rule.

However since beauty lies in the eye of beholder I'm fine with both :).

Lets come back ot this discussion a bit later when working on Rubocop rules

Copy link
Contributor Author

@delner delner Jun 22, 2018

Choose a reason for hiding this comment

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

For what its worth, the style choice here isn't arbitrary, it's an implementation of the http://www.betterspecs.org/ standard, which I think does a better job of explaining the reasoning than I can here.

This all comes down to style ultimately I think. Sometimes it's worth discussing the possible merits, and it's totally reasonable to suggest alternatives, but given that both suggested styles are clear and understandable, it doesn't seem like there's much to gain by dwelling too much on things like this. The same would apply if the circumstances were reversed, and it was the named example in the PR instead of the nameless one.

expect { log }.to output("Span#log is deprecated. Please use Span#log_kv instead.\n").to_stderr
end

it { is_expected.to be nil }
end

describe '#log_kv' do
subject(:log_kv) { span.log_kv(timestamp: timestamp, **fields) }
let(:timestamp) { Time.now }
let(:fields) { { time_started: Time.now, account_id: '1234' } }

before(:each) do
expect { log_kv }.to_not output.to_stderr
context 'when given arbitrary key/value pairs' do
let(:fields) { { time_started: Time.now, account_id: '1234' } }
# We don't expect this to do anything right now.
it { is_expected.to be nil }
end

it { is_expected.to be nil }
context 'when given an \'error.object\'' do
let(:fields) { { :'error.object' => error_object } }
let(:error_object) { instance_double(StandardError) }

before(:each) { expect(datadog_span).to receive(:set_error).with(error_object) }

it { is_expected.to be nil }
end
end
end
end