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

toddbeckman-wf · 2018-07-31T16:54:11Z

Note

Merges on #114 . Clean diff: tracing_for_lifecycle_transitions...AF-1540_consumer-specified-timing

Description

Allow consumers to specify when the module is actually in a useful state.

Testing

See the DataLoadAsync example and verify that the module_entered_first_useful_state span is created after the data is loaded. It should have a followsFrom reference to the load_module span with the same startTime and module.instanceId.

aviary-wf · 2018-07-31T16:54:42Z

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 HipChat: InfoSec Forum.

codecov-io · 2018-07-31T16:58:34Z

Codecov Report

Merging #115 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   96.67%   96.82%   +0.14%     
==========================================
  Files           3        4       +1     
  Lines         331      346      +15     
==========================================
+ Hits          320      335      +15     
  Misses         11       11

Impacted Files	Coverage Δ
lib/src/lifecycle_module.dart	`96.65% <100%> (+0.14%)`	⬆️
lib/src/timing_specifiers.dart	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd03bf4...4d40242. Read the comment docs.

toddbeckman-wf · 2018-08-01T21:29:52Z

lib/src/timing_specifiers.dart

@@ -0,0 +1,16 @@
+class StartupTimingSpecifier {


Not my favorite name.

Yeah I would kind of like to see it reference "mark" or "metric", but I can't think of anything good.

I was thinking "mark" or "indicate" for the verb for use in the method name, and then this could be "type" or un-suffixed or something.

markStartupTiming(StartupTiming.firstEditable); indicateStartupTiming(StartupTimingType.firstEditable);

evanweible-wf · 2018-08-06T22:07:38Z

lib/src/timing_specifiers.dart

+  static const StartupTimingSpecifier firstReadable =
+      const StartupTimingSpecifier._('module_entered_first_readable_state');
+
+  static const StartupTimingSpecifier firstUseful =


Do we have clear documentation around how these should be determined? How is useful different than editable+readable? or maybe even just readable?

This also seems to overlap with our Experience's onEnterIdleState event: https://github.com/Workiva/wdesk_sdk/blob/master/lib/src/experience_framework/experiences/experience.dart#L138

I'm assuming these would be preferred to that?

@evanweible-wf Yeah, I think the intention is to unify or replace that event with this API.

+1 we should have pretty solid use-cases and guidelines for each type before committing to adding them.

Thoughts on starting with just a constant for the equivalent to onEnterIdleState (i.e., AF-1540 and not AF-1541).

+1 to replacing that event.

The problem with just going with one option now and moving on is introducing a public API that we might change once we support other events. That was the main motivation to combining the PR's. I can undo it easily, but we should keep that in mind.

evanweible-wf · 2018-08-06T22:09:14Z

lib/src/timing_specifiers.dart

@@ -0,0 +1,16 @@
+class StartupTimingSpecifier {


Yeah I would kind of like to see it reference "mark" or "metric", but I can't think of anything good.

evanweible-wf · 2018-08-06T23:33:17Z

I'm a little concerned with how this overlaps with the existing idleState event from Experience, but as long as we have a plan and clear documentation on what should be used (and maybe deprecate idle state?), then I'm okay with this

greglittlefield-wf · 2018-08-07T23:18:28Z

example/panel/modules/data_load_async_module.dart

    _actions.loadData();
-    return new Future.value();
+    return null;


Why is this returning null instead of a future?

greglittlefield-wf · 2018-08-07T23:20:35Z

lib/src/lifecycle_module.dart

@@ -52,12 +54,21 @@ enum LifecycleState {
 /// Intended to be extended by most base module classes in order to provide a
 /// 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


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

Is it alright that this PR is already merging them?

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.

greglittlefield-wf · 2018-08-07T23:22:49Z

lib/src/lifecycle_module.dart

+        .startSpan(
+          specifier.name,
+          references: [tracer.followsFrom(_loadContext)],
+          startTime: _startLoadTime,


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

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

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

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

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.

greglittlefield-wf · 2018-08-07T23:27:20Z

lib/src/timing_specifiers.dart

@@ -0,0 +1,16 @@
+class StartupTimingSpecifier {


I was thinking "mark" or "indicate" for the verb for use in the method name, and then this could be "type" or un-suffixed or something.

markStartupTiming(StartupTiming.firstEditable); indicateStartupTiming(StartupTimingType.firstEditable);

greglittlefield-wf · 2018-08-07T23:31:14Z

lib/src/timing_specifiers.dart

+  static const StartupTimingSpecifier firstReadable =
+      const StartupTimingSpecifier._('module_entered_first_readable_state');
+
+  static const StartupTimingSpecifier firstUseful =


@evanweible-wf Yeah, I think the intention is to unify or replace that event with this API.

+1 we should have pretty solid use-cases and guidelines for each type before committing to adding them.

Thoughts on starting with just a constant for the equivalent to onEnterIdleState (i.e., AF-1540 and not AF-1541).

greglittlefield-wf · 2018-08-07T23:36:36Z

lib/src/lifecycle_module.dart

+    StartupTimingSpecifier specifier, {
+    Map<String, dynamic> tags: const {},
+  }) {
+    // Load didn't start


Should this throw a StateError instead of returning null?

greglittlefield-wf · 2018-08-07T23:38:15Z

lib/src/lifecycle_module.dart

+  @protected
+  void specifyStartupTiming(
+    StartupTimingSpecifier specifier, {
+    Map<String, dynamic> tags: const {},


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

There's already followsFrom reference to the load span.

greglittlefield-wf · 2018-08-07T23:38:43Z

lib/src/timing_specifiers.dart

@@ -0,0 +1,16 @@
+class StartupTimingSpecifier {


This class and its constants need doc comments

greglittlefield-wf · 2018-08-07T23:39:07Z

test/lifecycle_module_test.dart

@@ -281,15 +299,22 @@ void main() {
  Logger.root.level = Level.ALL;
  final StateError testError = new StateError('You should have expected this');

+  final TestTracer tracer = new TestTracer();
+  initGlobalTracer(tracer);
+  assert(globalTracer() == tracer);


Same comments as in the last PR around testing and whether a non-null globalTracer is required

The testing setup for this is being null or not-null pretty tricky. I'll think on it.

could do something like:

main() { + void allOfTheLifecycleModuleTests() { group('LifecycleModule', () { ... }); + } + + group('with test tracer set up', () { + setUp(() { + initGlobalTracer(new TestTracer()); + }); + + allOfTheLifecycleModuleTests(); + }); + + group('with a null global tracer', () { + setUp(() { + initGlobalTracer(null); + }); + + allOfTheLifecycleModuleTests(); + }); }

greglittlefield-wf · 2018-08-07T23:39:08Z

test/lifecycle_module_test.dart

@@ -281,15 +299,22 @@ void main() {
  Logger.root.level = Level.ALL;
  final StateError testError = new StateError('You should have expected this');

+  final TestTracer tracer = new TestTracer();
+  initGlobalTracer(tracer);
+  assert(globalTracer() == tracer);


Same comments as in the last PR around testing and whether a non-null globalTracer is required

lib/src/timing_specifiers.dart

lib/src/lifecycle_module.dart

greglittlefield-wf · 2018-08-27T18:34:29Z

lib/src/lifecycle_module.dart

+  }) {
+    // Load didn't start
+    if (_loadContext == null || _startLoadTime == null) {
+      return null;


This comment got collapsed without getting addressed:

Should this throw a StateError instead of returning null?

greglittlefield-wf · 2018-08-27T18:37:46Z

lib/src/timing_specifiers.dart

+  final String name;
+  const StartupTimingType._(this.name);
+
+  static const StartupTimingType firstComponentRender =


Continuing this unresolved conversation: #115 (comment)

The problem with just going with one option now and moving on is introducing a public API that we might change once we support other events. That was the main motivation to combining the PR's. I can undo it easily, but we should keep that in mind.

I don't think that removing all but one constant vs leaving them as-is puts us at any more risk for changing the public API.

This comment was in regards to having it be a specific 'useful state' method call versus this enumeration approach.

It's pretty trivial, if we go this enum route, to have just one vs all of them. I don't see the advantage of splitting them up into multiple tickets.

greglittlefield-wf · 2018-09-07T00:25:43Z

lib/src/timing_specifiers.dart

+  const StartupTimingType._(this.name);
+
+  /// Specifies the completion of the module's first render.
+  static const StartupTimingType firstComponentRender =


Continued conversation from #115 (comment)

@toddbeckman-wf Yeah I'm in agreement with the enum approach.

I'm just concerned about us knowing whether the meaning of the different enum values you have here are useful and well-defined. To @evanweible-wf's point here:

Do we have clear documentation around how these should be determined? How is useful different than editable+readable? or maybe even just readable?

I'm also a little confused by the difference between firstUseful/firstReadable/firstEditable

We may need more time to discuss, refine, and document how many values we want to have and what they all mean.

While we do that, is there a single value we can to start with, as an MVP?

toddbeckman-wf · 2018-09-07T17:37:07Z

Continuing the conversation here so it doesn't get collapsed in the code changes:

I am okay with starting with just one. The "first useful" state was the one we originally discussed in Hipchat as the first meaningful one, with AF-1541 giving more optional granularity, so that's the one I started with for now. It's easy enough to refactor to whichever other one we want if we change our minds.

toddbeckman-wf · 2018-09-10T15:54:29Z

lib/src/lifecycle_module.dart

+  ///
+  /// Any [tags] or [references] specified will be added to this span.
+  @protected
+  void specifyEnterFirstUsefulStateTiming({


Supercalifragilisticexpialidocious

Do we want to have shorthands for each one, or should we just have consumers use specifyStartupTiming(StartupTimingType.firstUseful)?

greglittlefield-wf

One question, but still looks good.

greglittlefield-wf · 2018-09-17T17:32:24Z

lib/src/lifecycle_module.dart

+  ///
+  /// Any [tags] or [references] specified will be added to this span.
+  @protected
+  void specifyEnterFirstUsefulStateTiming({


Do we want to have shorthands for each one, or should we just have consumers use specifyStartupTiming(StartupTimingType.firstUseful)?

toddbeckman-wf · 2018-09-18T18:03:28Z

Github doesn't let me respond to that comment so I'll just do it here

We can have shorthands if we want. I removed the others from this PR so there's nothing else to have shorthands for. However I'd be down for shortening the name by a few hundred characters. Kinda pointless shorthand if it's longer than without lol

greglittlefield-wf · 2018-10-09T22:02:12Z

Sorry, in that conversation I was suggesting just removing the shorthand.

greglittlefield-wf

Could use a little more coverage, otherwise looks good!

greglittlefield-wf · 2018-10-09T21:53:46Z

example/panel/modules/data_load_async_module.dart

@@ -42,6 +44,8 @@ class DataLoadAsyncModule extends Module {
  @protected
  Future<Null> onLoad() {
    // trigger non-blocking async load of data
+    _events.didLoadData.first
+        .then((_) => specifyStartupTiming(StartupTimingType.firstUseful));


This will complete with an error if the module is disposed before didLoadData is emitted.

Not critical to change since it's sample code, but we should probably set a good example.

Instead, you can do:

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

greglittlefield-wf · 2018-10-09T22:01:25Z

test/lifecycle_module_test.dart

+                  .listen(expectAsync1((span) {
+                expect(span.startTime, startTime);
+              })));
+              module.specifyStartupTiming(specifier);


We should have test coverage for custom and default tags/references passed into this. (same for any shorthand methods we have).

josephswanson-wf · 2018-10-10T19:50:46Z

smithy.yaml

@@ -1,8 +1,7 @@
 project: dart
 language: dart

-# dart 1.24.2
-runner_image: drydock-prod.workiva.net/workiva/smithy-runner-generator:203768
+runner_image: google/dart:1.24.3


may as well delete this file.. everything has been building on Workiva Build via the Dockerfile for quite some time
(I suspect Rosie is failing her dependency scan because of this not being an allowed Smithy runner image)

…sumer-specified-timing

evanweible-wf · 2018-10-11T16:08:46Z

example/panel/modules/data_load_async_module.dart

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


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

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

greglittlefield-wf

+10 w/ some #nits

greglittlefield-wf · 2018-10-12T20:52:05Z

documentation/tracing.md

+
+In cases like these, use `specifyStartupTiming`:
+
+```


#nit could be a Dart code block.

greglittlefield-wf · 2018-10-12T20:55:45Z

lib/src/lifecycle_module.dart

  }

  /// 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.instanceId': _instanceId,


#not I know this wasn't changed in this PR, but should this be .instance_id, not .instanceId?

greglittlefield-wf · 2018-10-12T20:57:58Z

example/panel/modules/data_load_async_module.dart

 class DataLoadAsyncStore extends Store {
  DataLoadAsyncActions _actions;
-  List<String> _data;
-  bool _isLoading;
+  DataLoadAsyncEvents _events;


#nit this is never disposed

greglittlefield-wf · 2018-10-12T21:23:48Z

example/panel/modules/data_load_async_module.dart

+    _store = new DataLoadAsyncStore(_actions, _events);
+    _components = new DataLoadAsyncComponents(_actions, _store);
+
+    [_actions, _events, _store].forEach(manageDisposable);


The actions and events classes aren't Disposables ☹️

type 'DataLoadAsyncActions' is not a subtype of type 'Disposable' of 'disposable' where DataLoadAsyncActions is from http://localhost:8087/panel/modules/data_load_async_module.dart Disposable is from package:w_common/src/common/disposable.dart

toddbeckman-wf · 2018-10-12T21:37:57Z

pubspec.yaml

-  dart_style: ^0.2.16
-  dartdoc: ^0.9.0
+  dart_style: ^1.0.7
+  dartdoc: ">=0.13.0 <0.16.0"


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

greglittlefield-wf

+10

toddbeckman-wf · 2018-10-15T15:25:45Z

@Workiva/release-management-p

toddbeckman-wf added the status: WIP label Jul 31, 2018

toddbeckman-wf requested a review from a team as a code owner July 31, 2018 16:54

toddbeckman-wf requested a review from a team July 31, 2018 16:54

rmconsole-wf added the Merge Requirements Unmet label Jul 31, 2018

toddbeckman-wf mentioned this pull request Jul 31, 2018

AF-1541 additional timing granularity #116

Closed

toddbeckman-wf commented Aug 1, 2018

View reviewed changes

toddbeckman-wf changed the title ~~AF-1540 consumer specified timing~~ AF-1540 + AF-1541 consumer specified timing Aug 1, 2018

toddbeckman-wf removed the status: WIP label Aug 6, 2018

evanweible-wf reviewed Aug 6, 2018

View reviewed changes

greglittlefield-wf requested changes Aug 7, 2018

View reviewed changes

greglittlefield-wf reviewed Aug 27, 2018

View reviewed changes

greglittlefield-wf reviewed Sep 7, 2018

View reviewed changes

toddbeckman-wf changed the title ~~AF-1540 + AF-1541 consumer specified timing~~ AF-1540 consumer-specified timing for "first useful state" Sep 7, 2018

toddbeckman-wf commented Sep 10, 2018

View reviewed changes

greglittlefield-wf approved these changes Sep 17, 2018

View reviewed changes

AF-1540 consumer-specified timing

b2d7551

toddbeckman-wf force-pushed the AF-1540_consumer-specified-timing branch from 911f867 to b2d7551 Compare October 9, 2018 19:53

greglittlefield-wf requested changes Oct 9, 2018

View reviewed changes

toddbeckman-wf added 6 commits October 10, 2018 11:15

AF-1540 test custom tags and refs with specified timing

89b1723

update smithy to 1.24.3

905a80c

leftover semicolon

aab0cae

use google dart image

3d3b87b

more image stuff

5fad098

install chrome in Dockerfile

51cea1f

toddbeckman-wf added 2 commits October 10, 2018 12:24

install xvfb

fc57bd0

anyElement is borked in chrome test

52793c3

greglittlefield-wf approved these changes Oct 10, 2018

View reviewed changes

josephswanson-wf reviewed Oct 10, 2018

View reviewed changes

toddbeckman-wf added 2 commits October 10, 2018 15:08

Merge branch 'master' of github.com:Workiva/w_module into AF-1540_con…

72adb27

…sumer-specified-timing

remove smithy

917660c

greglittlefield-wf approved these changes Oct 10, 2018

View reviewed changes

evanweible-wf reviewed Oct 11, 2018

View reviewed changes

josephswanson-wf mentioned this pull request Oct 11, 2018

Delete smithy.yaml #135

Closed

AF-2674 documentation for w_module tracing

18919d1

toddbeckman-wf changed the title ~~AF-1540 consumer-specified timing for "first useful state"~~ AF-1540 AF-2674 consumer-specified timing for "first useful state" Oct 11, 2018

greglittlefield-wf approved these changes Oct 12, 2018

View reviewed changes

AF-1540 address nits

2240934

greglittlefield-wf reviewed Oct 12, 2018

View reviewed changes

toddbeckman-wf added 2 commits October 12, 2018 15:25

actions arent disposable

b40c1f4

better style and fix Events not being Disposable

4d40242

toddbeckman-wf commented Oct 12, 2018

View reviewed changes

greglittlefield-wf approved these changes Oct 12, 2018

View reviewed changes

evanweible-wf approved these changes Oct 15, 2018

View reviewed changes

rmconsole-wf added RM Ready Merge Requirements Met and removed Merge Requirements Unmet labels Oct 15, 2018

rmconsole6-wk merged commit e780076 into master Oct 15, 2018

rmconsole6-wk deleted the AF-1540_consumer-specified-timing branch October 15, 2018 15:51

rmconsole-readonly-wk mentioned this pull request Oct 15, 2018

AF-2755 Release w_module 2.0.3 #136

Merged

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

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

Uh oh!

Conversation

toddbeckman-wf commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note

Description

Testing

Uh oh!

aviary-wf commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Insights

Action Items

Uh oh!

codecov-io commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanweible-wf commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddbeckman-wf Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

toddbeckman-wf commented Jul 31, 2018 •

edited

Loading

aviary-wf commented Jul 31, 2018 •

edited

Loading

codecov-io commented Jul 31, 2018 •

edited

Loading

toddbeckman-wf Aug 8, 2018 •

edited

Loading

toddbeckman-wf commented Sep 7, 2018 •

edited

Loading