Skip to content

AF-1540 AF-2674 consumer-specified timing for "first useful state" #115

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

Merged
merged 15 commits into from
Oct 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
title: w_module
base: github:Workiva/w_module/
src: README.md
src: README.md
topics:
- title: Tracing with w_module
src: docs/tracing.md
73 changes: 73 additions & 0 deletions documentation/tracing.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# Tracing with w_module

This library supports [OpenTracing][opentracingio] using [opentracing_dart][opentracingdart]. Your application will need to provide a `Tracer` and initialize it with `initGlobalTracer` to opt in to this feature.


To get the traces provided by this library, your module must provide a definition for the `name` getter. This can simply be the name of the class. For example:

```dart
class SomeModule extends Module {
@override
final String name = 'SomeModule';

// ... the of the module's implementation
}
```

Spans will be in the form of:

```
$name.$operationName
```

## Types Of Provided Traces

### Tracing Lifecycle Methods

We automically trace each of its lifecycle methods:

- Load
- Unload
- Suspend
- Resume

In addition, any spans created by child modules (loaded with `loadChildModule`) will have a `followsFrom` reference to the parent's span of the respective method to complete the story of the trace.

If you wish to create other `childOf` or `followsFrom` spans on your module's lifecycle spans, you can simply request the `activeSpan`:

```dart
@override
Future<Null> onLoad() {
// ... some loading logic

final span = globalTracer().startSpan(
operationName,
childOf: activeSpan.context, // see this line
);

// ... more loading logic
span.finish()
}
```

Note that `activeSpan` will be null at times when the module is not in the middle of a lifecycle transition.

### Additional Load Time Granularity

Sometimes, lifecycle methods such as `load` will complete before the module is semantically "loaded". For example, you may begin asynchronously fetching data for your module and then return from `onLoad` to keep from blocking the main thread.

In cases like these, use `specifyStartupTiming`:

```dart
Future<Null> onLoad() {
// ... kick off async loadData logic

listenToStream(_events.didLoadData.take(1),
(_) => specifyStartupTiming(StartupTimingType.firstUseful));
}
```

This will create a span starting from the same time as `load()` and ending at the moment the method was called. This library will handle the `operationName` and the `followsFrom` reference to the module's `load` span, but tags and references can be passed into this method just like with any other span in optional parameters.

[opentracingio]: https://opentracing.io/
[opentracingdart]: https://github.com/Workiva/opentracing_dart/
35 changes: 26 additions & 9 deletions example/panel/modules/data_load_async_module.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import 'dart:async';

import 'package:meta/meta.dart' show protected;
import 'package:react/react.dart' as react;
import 'package:w_common/disposable.dart';
import 'package:w_flux/w_flux.dart';
import 'package:w_module/w_module.dart';

Expand All @@ -27,12 +28,16 @@ class DataLoadAsyncModule extends Module {

DataLoadAsyncActions _actions;
DataLoadAsyncComponents _components;
DataLoadAsyncStore _stores;
DataLoadAsyncEvents _events;
DataLoadAsyncStore _store;

DataLoadAsyncModule() {
_actions = new DataLoadAsyncActions();
_stores = new DataLoadAsyncStore(_actions);
_components = new DataLoadAsyncComponents(_actions, _stores);
_events = new DataLoadAsyncEvents();
_store = new DataLoadAsyncStore(_actions, _events);
_components = new DataLoadAsyncComponents(_actions, _store);

<Disposable>[_events, _store].forEach(manageDisposable);
}

@override
Expand All @@ -42,6 +47,8 @@ class DataLoadAsyncModule extends Module {
@protected
Future<Null> onLoad() {
// trigger non-blocking async load of data
listenToStream(_events.didLoadData.take(1),
(_) => specifyStartupTiming(StartupTimingType.firstUseful));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it really matters, but just for my own clarification, this could just be specifyFirstUsefulState() right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, either way works. We weren't decided on whether we wanted shorthand at the time the example was written.

_actions.loadData();
return new Future.value();
}
Expand All @@ -62,15 +69,23 @@ class DataLoadAsyncActions {
final Action loadData = new Action();
}

DispatchKey _dispatchKey = new DispatchKey('DataLoadAsync');

class DataLoadAsyncEvents extends EventsCollection {
final Event didLoadData = new Event(_dispatchKey);
DataLoadAsyncEvents() : super(_dispatchKey) {
manageEvent(new Event(_dispatchKey));
}
}

class DataLoadAsyncStore extends Store {
DataLoadAsyncActions _actions;
List<String> _data;
bool _isLoading;
DataLoadAsyncEvents _events;
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit this is never disposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

List<String> _data = [];
bool _isLoading = false;

DataLoadAsyncStore(this._actions) {
_isLoading = false;
_data = [];
triggerOnAction(_actions.loadData, _loadData);
DataLoadAsyncStore(this._actions, this._events) {
manageActionSubscription(_actions.loadData.listen(_loadData));
}

/// Public data
Expand All @@ -89,6 +104,8 @@ class DataLoadAsyncStore extends Store {
// trigger on return of final data
_data = ['Aaron', 'Dustin', 'Evan', 'Jay', 'Max', 'Trent'];
_isLoading = false;
_events.didLoadData(null, _dispatchKey);
trigger();
}
}

Expand Down
3 changes: 1 addition & 2 deletions example/panel/modules/sample_tracer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,7 @@ class SampleTracer implements AbstractTracer {
references: references,
startTime: startTime,
tags: tags,
)
..whenFinished.then((span) {
)..whenFinished.then((span) {
print(span.toString());
});
}
Expand Down
87 changes: 76 additions & 11 deletions lib/src/lifecycle_module.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import 'package:opentracing/opentracing.dart';
import 'package:w_common/disposable.dart';

import 'package:w_module/src/simple_module.dart';
import 'package:w_module/src/timing_specifiers.dart';

/// Possible states a [LifecycleModule] may occupy.
enum LifecycleState {
Expand Down Expand Up @@ -54,7 +55,7 @@ enum LifecycleState {
/// unified lifecycle API.
abstract class LifecycleModule extends SimpleModule with Disposable {
static int _nextId = 0;

// Used by tracing to tell apart multiple instances of the same module
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this and other comments need to make their way into the lifecycle tracing PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it alright that this PR is already merging them?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if they were in the other PR so that work could be reviewed atomically, but it's fine it it's too much effort to make that happen.

int _instanceId = _nextId++;

List<LifecycleModule> _childModules = [];
Expand All @@ -64,7 +65,10 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
LifecycleState _state = LifecycleState.instantiated;
Completer<Null> _transition;
Span _activeSpan;
SpanContext _loadSpanContext;

// Used by tracing to create a span if the consumer specifies when the module
// reaches its first useful state
DateTime _startLoadTime;

// Lifecycle event StreamControllers
StreamController<LifecycleModule> _willLoadChildModuleController =
Expand Down Expand Up @@ -116,9 +120,7 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
_didUnloadController,
].forEach(manageStreamController);

<
String,
Stream>{
<String, Stream>{
'willLoad': willLoad,
'didLoad': didLoad,
'willLoadChildModule': willLoadChildModule,
Expand Down Expand Up @@ -153,6 +155,10 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
@protected
Span get activeSpan => _activeSpan;

/// Set internally by this module for the load span so it can be used as a
/// `Reference` to other spans after the span is finished.
SpanContext _loadContext;

/// Set internally by the parent module if this module is called by [loadChildModule]
SpanContext _parentContext;

Expand All @@ -177,16 +183,74 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
references.add(new Reference.followsFrom(_parentContext));
}

return tracer.startSpan('$name.$operationName', references: references)
..addTags({
'module.instanceId': _instanceId,
});
return tracer.startSpan(
'$name.$operationName',
references: references,
tags: _defaultTags,
);
}

/// Creates a span with `globalTracer` from the start of [load] until now.
///
/// This span is intended to represent the time it takes for the module to
/// finish asynchronously loading any necessary data and entering a state which
/// is ready for user interaction.
///
/// Any [tags] or [references] specified will be added to this span.
@protected
void specifyFirstUsefulState({
Map<String, dynamic> tags: const {},
List<Reference> references: const [],
}) =>
specifyStartupTiming(
StartupTimingType.firstUseful,
tags: tags,
references: references,
);

/// Creates a span with `globalTracer` from the start of [load] until now.
///
/// The [specifier] indicates the purpose of this span.
///
/// Any [tags] or [references] specified will be added to this span.
@protected
void specifyStartupTiming(
StartupTimingType specifier, {
Map<String, dynamic> tags: const {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we allow passing in other references so this reference can be associated with whatever triggered it, if that thing is traced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already followsFrom reference to the load span.

List<Reference> references: const [],
}) {
// Load didn't start
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this throw a StateError instead of returning null?

if (_loadContext == null || _startLoadTime == null) {
throw new StateError(
'Calling `specifyStartupTiming` before calling `load()`');
}

final tracer = globalTracer();
if (tracer == null) {
return null;
}

tracer
.startSpan(
'$name.${specifier.operationName}',
references: [tracer.followsFrom(_loadContext)]..addAll(references),
startTime: _startLoadTime,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we started this span at the same time as load as opposed to passing in the start time so that we could tie spans into browser marks/measures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we would end up with unfinished spans for all modules that don't implement closing these.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, didn't think about that...

...Is it a problem to have unfinished spans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each unfinished span leaks a Completer in the span object and a whenFinished stream listener in the tracer (App Intelligence for us). A few might go unnoticed, but I'm not comfortable with the possibility that this will leak memory over time if we're automatically starting them for every module and only finishing them for the handful that actually implement stopping these.

tags: _defaultTags..addAll(tags),
)
.finish();

_startLoadTime = null;
}

/// Name of the module for identification in exceptions and debug messages.
// ignore: unnecessary_getters_setters
String get name => _defaultName;

Map<String, dynamic> get _defaultTags => {
'span.kind': 'client',
'module.instance_id': _instanceId,
};

/// Deprecated: the module name should be defined by overriding the getter in
/// a subclass and it should not be mutable.
@deprecated
Expand Down Expand Up @@ -377,7 +441,8 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
}

_activeSpan = _startTransitionSpan('load');
_loadSpanContext = _activeSpan?.context;
_loadContext = _activeSpan?.context;
_startLoadTime = _activeSpan?.startTime;

_state = LifecycleState.loading;

Expand Down Expand Up @@ -455,7 +520,7 @@ abstract class LifecycleModule extends SimpleModule with Disposable {

try {
_childModules.add(childModule);
childModule._parentContext = _loadSpanContext;
childModule._parentContext = _loadContext;

await childModule.load();
await onDidLoadChildModule(childModule);
Expand Down
11 changes: 11 additions & 0 deletions lib/src/timing_specifiers.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// The type of 'startup timing metric' to be used by `specifyStartupTiming`
class StartupTimingType {
/// The `operationName` to be used for spans created using this [StartupTimingType].
final String operationName;

const StartupTimingType._(this.operationName);

/// Specifies that the module finished loading necessary data and is ready for user interaction.
static const StartupTimingType firstUseful =
const StartupTimingType._('entered_first_useful_state');
}
1 change: 1 addition & 0 deletions lib/w_module.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ export 'package:w_module/src/events_collection.dart';
export 'package:w_module/src/lifecycle_module.dart' hide LifecycleState;
export 'package:w_module/src/module.dart';
export 'package:w_module/src/simple_module.dart';
export 'package:w_module/src/timing_specifiers.dart';
6 changes: 3 additions & 3 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@ homepage: https://github.com/Workiva/w_module
dependencies:
logging: ^0.11.0
meta: ^1.0.0
opentracing: ^0.2.1
opentracing: ^0.2.3
platform_detect: ^1.1.0
w_common: ^1.9.0

dev_dependencies:
browser: ^0.10.0+2
coverage: ^0.7.3
dart_dev: ^1.8.0
dart_style: ^0.2.16
dartdoc: ^0.9.0
dart_style: ^1.0.7
dartdoc: ">=0.13.0 <0.16.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from wdesk_sdk. Shouldn't affect OSS consumers since it's a dev dep

mockito: ^1.0.1
react: ^3.0.0
test: ^0.12.0
Expand Down
14 changes: 0 additions & 14 deletions smithy.yaml

This file was deleted.

Loading