O11Y-988: Record Spans #2

tylersnavely-wf · 2021-08-03T16:06:44Z

This PR allows for creation of traces and their spans. A version is not being cut because this should not be consumed; spans are just being held in memory, so this could cause a problem if is was. The next bit of work would be to allow exporters that can properly manage the generated spans.

An example for how this works is as follows:

import 'package:opentelemetry/api.dart';
import 'package:opentelemetry/sdk.dart' as otel_sdk;

final Tracer tracer = otel_sdk.TracerProvider().getTracer('appName', version: '1.0.0');

doWork() {
  Span parent = getSpan(Context.current);

  withContext(setSpan(Context.current, parent), () {
    Span span = tracer.startSpan('doWork');
    ...
    span.end();
  });
}

doMoreWork() async {
  Span parent = getSpan(Context.current);

  await withContext(setSpan(Context.current, parent), () async {
    Span span = tracer.startSpan('doMoreWork');
    ...
    span.end();
  });
}

main() async {
  // parent span
  Span span = tracer.startSpan('work-setup');

  // force all enclosed new spans will be a child of the parent span.
  withContext(setSpan(Context.current, span), () {
    doWork();
  });

  await withContext(setSpan(Context.current, span), () async {
    await doMoreWork();
  });

  span.end();
}

rmconsole-wf · 2021-08-03T16:06:57Z

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

General Information

Ticket(s):

O11Y-988 | remove

Code Review(s): #2
Release Image Tags:

Reviewers: blakeroberts-wk, tylersnavely-wf

Additional Information

Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Version: opentelemetry-dart 0.1.0
	Release Ticket(s): None

Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Tuesday, August 10 11:58 AM CST

aviary2-wf · 2021-08-03T16:08:30Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

lib/api.dart

lib/src/api/context/context.dart

lib/src/api/trace/context_utils.dart

lib/src/api/trace/tracer.dart

pubspec.yaml

blakeroberts-wk · 2021-08-05T18:27:06Z

lib/api.dart

+    withContext;
+export 'src/api/trace/id_generator.dart'
+  show
+    IdGenerator;


does this need to be exposed?

like it is the api, but can the sdk consume this without exporting it publicly? More over, is this truly apart of the specification, or an implementation detail about which the specification dictates that a trace/span id be of a certain form?

Although I don't see anyone needing to, yes, according to the spec.
https://github.com/open-telemetry/opentelemetry-specification/blob/7cef355a15c4430aa323d155206a2945fcd67a05/specification/trace/sdk.md#id-generators

We provide a default ID generator, but to be fully compliant, we need to allow others to configure their own ID generators on their tracers. Exporting the abstract IdGenerator allows them to do so.

tylersnavely-wf · 2021-08-10T16:57:43Z

QA +1

tylersnavely-wf · 2021-08-10T16:58:34Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

evanweible-wf

Sorry for the post-merge review, I was out on some PTO and just got to this

evanweible-wf · 2021-08-10T17:21:48Z

lib/src/api/trace/context_utils.dart

+final ContextKey spanKey = Context.createKey('OpenTelemetry Context Key SPAN');
+
+/// Get the [Span] attached to the [context].
+dynamic getSpan(Context context) => context.getValue(spanKey);


Could this return type be Span?

evanweible-wf · 2021-08-10T17:22:20Z

lib/src/api/trace/context_utils.dart

+
+/// Get the SpanContext of the [context].
+dynamic getSpanContext(Context context) {
+  final span = context.getValue(spanKey);


Could we use getSpan() here?

Suggested change

final span = context.getValue(spanKey);

final span = getSpan(context);

evanweible-wf · 2021-08-10T17:24:29Z

lib/src/api/trace/context_utils.dart

+dynamic withContext(Context context, Function fn) {
+  final scope = Context.attach(context);
+  final result = fn();
+  context.detach(scope);
+  return result;


Based on the readme, it looks like this should support async functions, but currently this isn't awaiting that future at all, which would result in the scope being detached before it's complete. We'd need to check if result is Future like so:

Suggested change

dynamic withContext(Context context, Function fn) {

final scope = Context.attach(context);

final result = fn();

context.detach(scope);

return result;

dynamic withContext(Context context, Function fn) {

final scope = Context.attach(context);

var result = fn();

if (result is Future) {

result.whenComplete(() {

context.detach(scope);

});

} else {

context.detach(scope);

}

return result;

evanweible-wf · 2021-08-10T17:26:16Z

lib/src/api/trace/span.dart

+  /// Get the time when the span was closed, or null if still open.
+  int get endTime;
+
+  /// Get the time when the span was started.
+  int get startTime;


https://dart.dev/guides/language/effective-dart/documentation#prefer-starting-variable-getter-or-setter-comments-with-noun-phrases

The "Get " part of these doc comments could be omitted.

evanweible-wf · 2021-08-10T17:26:25Z

lib/src/api/trace/span_context.dart

+  /// Get the ID of the span.
+  String get spanId;
+
+  /// Get the ID of the trace the span is a part of.
+  String get traceId;
+
+  /// Get the state of the entire trace.
+  TraceState get traceState;


Same here: https://dart.dev/guides/language/effective-dart/documentation#prefer-starting-variable-getter-or-setter-comments-with-noun-phrases

evanweible-wf · 2021-08-10T18:01:16Z

test/integration/api/context_utils_test.dart

+  });
+
+  test('withContext attaches and detaches with async fn', () async {
+    expect(await withContext(Context.current, () async => 42), equals(42));


This test could be improved to verify that it is actually awaiting the async function given to it before detaching the scope. One way to do that might be like so:

Suggested change

expect(await withContext(Context.current, () async => 42), equals(42));

final key = Context.createKey('test');

final parentContext = Context.current;

final childContext = parentContext.setValue(key, 'foo');

final result = withContext(childContext, expectAsync0(() async {

// Verify that our context was attached prior to calling our function

expect(Context.current, same(childContext));

await Future.delayed(Duration(milliseconds: 100));

return 42;

}));

expect(await result, 42);

// Now that our function has completed, the child context should no longer be active

expect(Context.current, same(parentContext));

It might also be worth updating the sync test in a similar manner, such that it verifies that the expected context is active from within the callback.

This also made me wonder how withContext is expected to work in the case of multiple, concurrent async usages. Since Context.current is effectively a global singleton right now.. couldn't two async usages of withCurrent cause unexpected results where the second invocation could change the current context before the first invocation's async callback has completed?

Makes me wonder if the current scope should be read/written via a Zone value so that multiple context "branches" could exist concurrently.

evanweible-wf · 2021-08-10T18:06:21Z

test/integration/sdk/tracer_test.dart

+
+    final span = tracer.startSpan('foo');
+
+    expect(span.startTime, isA<int>());


Span.startTime is already typed as int, so this assertion may be confusing to those who read it. Maybe we just want to verify that it's non-null?

Suggested change

expect(span.startTime, isA<int>());

expect(span.startTime, isNotNull);

evanweible-wf · 2021-08-10T18:07:22Z

test/integration/sdk/tracer_test.dart

+    expect(span.spanContext.traceId, isA<String>());
+    expect(span.spanContext.spanId, isA<String>());


Same note here - this typing is already enforced by the language, so it's probably more appropriate to use isNotNull if that's what we're really wanting to verify here.

evanweible-wf · 2021-08-10T18:07:33Z

test/integration/sdk/tracer_test.dart

+
+    final childSpan = tracer.startSpan('bar', context: context);
+
+    expect(childSpan.startTime, isA<int>());


Suggested change

expect(childSpan.startTime, isA<int>());

expect(childSpan.startTime, isNotNull);

evanweible-wf · 2021-08-10T18:08:07Z

test/integration/sdk/tracer_test.dart

+    expect(childSpan.spanContext.spanId, allOf([
+      isA<String>(),
+      isNot(equals(parentSpan.spanContext.spanId))
+    ]));


Suggested change

expect(childSpan.spanContext.spanId, allOf([

isA<String>(),

isNot(equals(parentSpan.spanContext.spanId))

]));

expect(childSpan.spanContext.spanId, isNot(parentSpan.spanContext.spanId));

rmconsole-wf added the Merge Requirements Unmet label Aug 3, 2021

evanweible-wf reviewed Aug 3, 2021

View reviewed changes

O11Y-988: Record spans

b723b76

tylersnavely-wf force-pushed the create-spans branch from 48722d1 to b723b76 Compare August 5, 2021 16:27

tylersnavely-wf changed the title ~~create spans wip~~ O11Y-988: Record Spans Aug 5, 2021

tylersnavely-wf marked this pull request as ready for review August 5, 2021 16:41

tylersnavely-wf requested a review from a team as a code owner August 5, 2021 16:41

blakeroberts-wk reviewed Aug 5, 2021

View reviewed changes

blakeroberts-wk approved these changes Aug 5, 2021

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Aug 5, 2021

rmconsole-wf added the RM Ready label Aug 10, 2021

rmconsole7-wk merged commit e51f4eb into master Aug 10, 2021

rmconsole-wf approved these changes Aug 10, 2021

View reviewed changes

rmconsole7-wk deleted the create-spans branch August 10, 2021 16:58

evanweible-wf reviewed Aug 10, 2021

View reviewed changes

evanweible-wf mentioned this pull request Oct 18, 2024

PPI-203 : deprecate context manager and implement attach/detach APIs #192

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O11Y-988: Record Spans #2

O11Y-988: Record Spans #2

tylersnavely-wf commented Aug 3, 2021 •

edited

Loading

rmconsole-wf commented Aug 3, 2021 •

edited by rmconsole7-wk

Loading

aviary2-wf commented Aug 3, 2021

blakeroberts-wk Aug 5, 2021

blakeroberts-wk Aug 5, 2021

tylersnavely-wf Aug 5, 2021

tylersnavely-wf commented Aug 10, 2021

tylersnavely-wf commented Aug 10, 2021

rmconsole-wf left a comment

evanweible-wf left a comment

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

evanweible-wf Aug 10, 2021

	final span = context.getValue(spanKey);
	final span = getSpan(context);

-    expect(await withContext(Context.current, () async => 42), equals(42));
+    final key = Context.createKey('test');
+    final parentContext = Context.current;
+    final childContext = parentContext.setValue(key, 'foo');
+    final result = withContext(childContext, expectAsync0(() async {
+      // Verify that our context was attached prior to calling our function
+      expect(Context.current, same(childContext));
+      await Future.delayed(Duration(milliseconds: 100));
+      return 42;
+    }));
+    expect(await result, 42);
+    // Now that our function has completed, the child context should no longer be active
+    expect(Context.current, same(parentContext));


		final span = tracer.startSpan('foo');

		expect(span.startTime, isA<int>());

	expect(span.startTime, isA<int>());
	expect(span.startTime, isNotNull);

		expect(span.spanContext.traceId, isA<String>());
		expect(span.spanContext.spanId, isA<String>());


		final childSpan = tracer.startSpan('bar', context: context);

		expect(childSpan.startTime, isA<int>());

	expect(childSpan.startTime, isA<int>());
	expect(childSpan.startTime, isNotNull);

O11Y-988: Record Spans #2

O11Y-988: Record Spans #2

Conversation

tylersnavely-wf commented Aug 3, 2021 • edited Loading

rmconsole-wf commented Aug 3, 2021 • edited by rmconsole7-wk Loading

Merge Requirements Met ✅

General Information

Additional Information

aviary2-wf commented Aug 3, 2021

Security Insights

Action Items

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylersnavely-wf commented Aug 10, 2021

tylersnavely-wf commented Aug 10, 2021

rmconsole-wf left a comment

Choose a reason for hiding this comment

evanweible-wf 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylersnavely-wf commented Aug 3, 2021 •

edited

Loading

rmconsole-wf commented Aug 3, 2021 •

edited by rmconsole7-wk

Loading