[OpenTracing] Span and SpanContext implementation #472

delner · 2018-06-20T21:32:34Z

This pull request implements the OpenTracing Span and SpanContext. In their current forms, they are very simple, and may not represent the full scope of what will ultimately be supported.

Some worthwhile notes:

The constructors for Span and SpanContext are still somewhat undetermined. They may require changes depending on how and where we actually construct and configure spans. The current plan is that they are treated like simple data structs, that are not responsible for building sophisticated Spans or SpanContexts. That logic is anticipated to be a part of the Tracer, or some kind of factory pattern.
There's some ambiguity as to how log and log_kv should be implemented. For now, they simply set errors, if provided with one. Otherwise they do nothing. We didn't interpret these functions to have responsibilities beyond setting some "debug tags", as opposed to doing logging in the traditional sense.
We might want to add span_id and trace_id to the SpanContext, since this object is meant to be a serializable layer of data for sharing trace data across process boundaries (particularly for the purposes of distributed tracing.)

Kyle-Verhoog

Just a couple of things.

Kyle-Verhoog · 2018-06-21T15:58:19Z

lib/ddtrace/opentracer/span.rb

+      # @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 }


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.

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?

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

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).

@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.

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

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

Kyle-Verhoog · 2018-06-21T16:04:28Z

lib/ddtrace/opentracer/span.rb

+      # 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)


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

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.

pawelchcki

One tiny comment otherwise LGTM!

pawelchcki · 2018-06-21T16:12:58Z

spec/ddtrace/opentracer/span_spec.rb

@@ -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


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

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.

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

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.

…dog::Context.

Kyle-Verhoog

Looks good.

Kyle-Verhoog · 2018-06-22T20:27:07Z

lib/ddtrace/opentracer/span.rb

+        tap do
+          # SpanContext is immutable, so to make changes
+          # build a new span context.
+          @span_context = SpanContextFactory.clone(


Kyle-Verhoog · 2018-06-22T20:29:34Z

lib/ddtrace/opentracer/span_context.rb

+        :datadog_context
+
+      def initialize(datadog_context:, baggage: {})
+        @datadog_context = datadog_context


* Added: Datadog::OpenTracer::SpanContext implementation. * Added: Datadog::OpenTracer::Span implementation. * Changed: Bump required Ruby version for OpenTracing to 2.1. * Added: Datadog::OpenTracer::SpanContextFactory. * Changed: OpenTracer::Span#set_baggage_item to create new SpanContexts. * Changed: Datadog::OpenTracer::SpanContext#baggage to be immutable. * Added: span_id, trace_id, parent_id to Datadog::OpenTracer::SpanContext. * Changed: Replaced span_id, trace_id, parent_id with reference to Datadog::Context.

delner added 3 commits June 20, 2018 17:13

Added: Datadog::OpenTracer::SpanContext implementation.

04abcab

Added: Datadog::OpenTracer::Span implementation.

f8910b6

Changed: Bump required Ruby version for OpenTracing to 2.1.

c6d3007

delner added core Involves Datadog core libraries feature Involves a product feature labels Jun 20, 2018

delner self-assigned this Jun 20, 2018

delner requested review from pawelchcki and Kyle-Verhoog June 20, 2018 21:32

Kyle-Verhoog requested changes Jun 21, 2018

View reviewed changes

pawelchcki requested changes Jun 21, 2018

View reviewed changes

delner added 4 commits June 21, 2018 16:00

Added: Datadog::OpenTracer::SpanContextFactory.

cdb7f4b

Changed: OpenTracer::Span#set_baggage_item to create new SpanContexts.

a8ca2e1

Changed: Datadog::OpenTracer::SpanContext#baggage to be immutable.

65915bf

Added: span_id, trace_id, parent_id to Datadog::OpenTracer::SpanContext.

8e369d5

pawelchcki approved these changes Jun 22, 2018

View reviewed changes

Changed: Replaced span_id, trace_id, parent_id with reference to Data…

6f16be3

…dog::Context.

Kyle-Verhoog approved these changes Jun 22, 2018

View reviewed changes

delner merged commit 6c09b63 into feature/opentracing Jun 22, 2018

delner deleted the feature/opentracing_span_and_span_context branch August 14, 2018 17:43

delner added this to the 0.16.0 milestone Aug 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OpenTracing] Span and SpanContext implementation #472

[OpenTracing] Span and SpanContext implementation #472

delner commented Jun 20, 2018

Kyle-Verhoog left a comment

Kyle-Verhoog Jun 21, 2018

delner Jun 21, 2018

Kyle-Verhoog Jun 21, 2018

Kyle-Verhoog Jun 21, 2018 •

edited

Loading

delner Jun 21, 2018

Kyle-Verhoog Jun 21, 2018

delner Jun 21, 2018

Kyle-Verhoog Jun 21, 2018

delner Jun 21, 2018

pawelchcki left a comment

pawelchcki Jun 21, 2018

delner Jun 21, 2018 •

edited

Loading

pawelchcki Jun 22, 2018

delner Jun 22, 2018 •

edited

Loading

Kyle-Verhoog left a comment

Kyle-Verhoog Jun 22, 2018

Kyle-Verhoog Jun 22, 2018

[OpenTracing] Span and SpanContext implementation #472

[OpenTracing] Span and SpanContext implementation #472

Conversation

delner commented Jun 20, 2018

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Verhoog Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelchcki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Verhoog Jun 21, 2018 •

edited

Loading

delner Jun 21, 2018 •

edited

Loading

delner Jun 22, 2018 •

edited

Loading