Adding cancellation reason diagnostics #77
Conversation
There was a problem hiding this comment.
A couple initial thoughts:
FIrst, I just want to make sure you saw my prior attempt at this here? #67
Second, I did notice that you ended up gravitating to a very similar solution: two sets of subscribers, one for measurements and another for cancellations. That said, I was annoyed that my solution required breaking away from the standard observable interface. If we can come up with anything that better matches this, I'd be a lot happier.
Third, assuming we do go with two separate subscriptions, I really like that your solution is a lot simpler! It makes total sense that you can just report the cancellation immediately instead of storing it in state somewhere like I was trying to do. 👍
And finally, I'd like to introduce some test coverage and documentation for a change like this. You might be able to copy-paste what I have in the first diff as a starting point! Whoops, I just noticed you already have this planned in your initial comment!
kierrrrra
force-pushed
the
kada/debug-cancellation-reason
branch
2 times, most recently
from
d88eab1
to
4f56feb
Compare
kierrrrra
changed the title
kierrrrra
force-pushed
the
kada/debug-cancellation-reason
branch
from
4f56feb
to
598ff79
Compare
src/visuallyCompleteCalculator.ts
Outdated
| export type Metric = CommonMetadata & { | ||
| // value of "TTVC" | ||
| duration: number; | ||
| }; | ||
|
|
||
| // currently, the only source of error is cancellation | ||
| export type CancellationError = CommonMetadata & { | ||
| // reason for cancellation | ||
| cancellationReason: CancellationReason; | ||
|
|
||
| // Optional type of event that triggered cancellation | ||
| eventType?: string; | ||
|
|
||
| // Optional target of event that triggered cancellation | ||
| eventTarget?: EventTarget; | ||
| }; |
There was a problem hiding this comment.
Hmm, what do you think about trying to make the CancellationError type also conform to the PerformanceMeasure specification?
I think the main change would be ensuring that all metadata (including cancellationReason goes into the detail bucket.
Nit: I would also just as soon not try to share a CommonMetadata interface for these two results. Personally, I think it's a little more awkward to read and doesn't save that many characters. 😅
There was a problem hiding this comment.
I don't see a reason to align that to PerformanceMeasure, but if you feel strongly about it, I can move the data into detail to make it conform.
As for CommonMetadata - I actually find it easier to read that way. When I scan the code, I can quickly see CommonMetadata shared between the types and it saves me the mental cycles on thinking whether the two are actually alike or not.
There was a problem hiding this comment.
I put some more thought into it, and I think it might be awkward to try to conform to PerformanceMeasure for something that is not a measure of time, but a record of occurrence. I would like to keep it the way it is - or have a deeper conversation about where we see value in having error state conform to PerformanceMeasure.
There was a problem hiding this comment.
Okay, I'm pretty convinced by your rationale for the cancellation type diverging from PerformanceMeasure schema! 👍
However, I think that makes me want to push a little further on the awkwardness of CommonMetadata. I would like it to be clearer that Metric is a data type that is designed to look similar to PerformanceMeasure and CancellationError is not.
Maybe we could change the structure so that no fields are nested in detail in CancellationError? Another small bonus of that change would be that didNetworkTimeOut?: boolean; can be explicit again; it's always present in a Metric and only sometimes present in a CancellationError.
There was a problem hiding this comment.
Of course - that sounds good to me! I refactored the types to be distinctly separate, and added a note on Metric's alignment with PerformanceMeasure for future reference. Thank you Andrew!
README.md
Outdated
| detail: { | ||
| // ... identical to `detail` property of Metric type ... | ||
| }; |
There was a problem hiding this comment.
Last nit: It looks like this section in the README is stale now.
Why
We want to log the reason why TTVC measurement was cancelled. The most accurate way would be to implement that functionality in the library itself, especially considering that we will add more diagnostic information down the line.
What
This implements a simple interface to add listeners to diagnostic events.
Testing
interaction1e2e test case, performing a click/keydown/tab switch while the page is still loading. Diagnostic with the reason for measurement cancellation will be logged as a warning to the console.