1530133 - Update messages shown according to the currently enabled logpoints

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

When a logpoint is added while replaying, it applies retroactively throughout the recording. This is kind of problematic when logpoints are being edited or frequently added and removed, as the console will become populated with lots of old irrelevant messages.

It would be better if the logpoint messages shown in the console reflected those which are currently installed. The attached patch makes this change, and a couple others:

• Adds support for conditional logpoints when replaying.

• Adds tests for the new and existing logpoint functionality.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Debugger changes for removing obsolete log points.

The debugger client removes old logpoints by first generating a uniqueish ID (via Math.random) every time it sets a logpoint in a replaying process. These IDs are associated with each message sent to the console for that logpoint, and when the logpoint is removed by the client it sends an event to the console to remove all messages associated with that ID.

Note that right now this functionality only works with web replay. I think we'll want to do something like this for normal logpoints at some point, but figuring out the right semantics is a little trickier, see https://github.com/firefox-devtools/debugger/issues/7816

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Allow clearing console messages by their logpoint ID.

Keep track of the optional logpoint ID in console messages, and add a MESSAGES_CLEAR_LOGPOINT redux action which the debugger invokes to remove all console messages with a given logpoint ID.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Support conditional logpoints when replaying.

This is largely an independent change (part 1 had to touch the calling code so I thought we might as well fix this now), but this patch changes the ReplayDebuggerScript.replayVirtualConsoleLog interface to support conditional logpoints (which we already support in the normal forward-execution case).

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Add logpoint tests.

Add a couple new tests for logpoint functionality when replaying. This also removes the boilerplate in the test files describing how to disable the web replay tests, which isn't relevant now that the tests are tier 2.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9046176 [details] [diff] [review]
Part 2 - Allow clearing console messages by their logpoint ID.

Review of attachment 9046176 [details] [diff] [review]:
-----------------------------------------------------------------

Hello Brian, thanks for the patch. I'm really sorry about the long delay :/

So, I have a couple of nits and one question about the structure to store the removed logPoints.
Also, I think we should release actors when removing the logPoints. You can see how we do it in [devtools/client/webconsole/enhancers/actor-releaser.js#19-29](https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/devtools/client/webconsole/enhancers/actor-releaser.js#19-29). `state.messages.removedActors` is populated in `removeMessagesFromState`, which you use, so what's left to do is add check the clear log point action in actor-releaser.js .

Out of curiosity, when does `clearLogpointMessages` is emitted?

::: devtools/client/webconsole/reducers/messages.js
@@ +58,5 @@
>    // Map of the form {messageId : networkInformation}
>    // `networkInformation` holds request, response, totalTime, ...
>    networkMessagesUpdateById: {},
> +  // Set of logpoint IDs that have been removed
> +  removedLogpointIds: {},

nit: could it be `removedLogPointIds`?

@@ +269,5 @@
> +
> +      return removeMessagesFromState({
> +        ...state,
> +        removedLogpointIds: { ...state.removedLogpointIds, [action.logpointId]: true },
> +      }, removedIds);

could `removeLogPointsIds` be an Set?
It doesn't feel like we use it for anything else than a lookup (on line 113), so a Set could be good?

::: devtools/client/webconsole/utils/messages.js
@@ +190,4 @@
>      prefix: message.prefix,
>      private: message.private,
>      executionPoint: message.executionPoint,
> +    logpointId: message.logpointId,

nit: could it be `logPointId`?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Nicolas Chevobbe from comment #5)

Comment on attachment 9046176 [details] [diff] [review]
Part 2 - Allow clearing console messages by their logpoint ID.

Review of attachment 9046176 [details] [diff] [review]:

Hello Brian, thanks for the patch. I'm really sorry about the long delay :/

So, I have a couple of nits and one question about the structure to store
the removed logPoints.
Also, I think we should release actors when removing the logPoints. You can
see how we do it in
[devtools/client/webconsole/enhancers/actor-releaser.js#19-29](https://
searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/
devtools/client/webconsole/enhancers/actor-releaser.js#19-29).
state.messages.removedActors is populated in removeMessagesFromState,
which you use, so what's left to do is add check the clear log point action
in actor-releaser.js .

Out of curiosity, when does clearLogpointMessages is emitted?

Calls to emit are in part 1. When the debugger client removes or changes a breakpoint with a logpoint value in the server then it will (if we are replaying) emit clearLogpointMessages to remove messages associated with the old logpoint value.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Nicolas Chevobbe from comment #5)

::: devtools/client/webconsole/reducers/messages.js
@@ +58,5 @@

// Map of the form {messageId : networkInformation}
// networkInformation holds request, response, totalTime, ...
networkMessagesUpdateById: {},

• // Set of logpoint IDs that have been removed
• removedLogpointIds: {},

nit: could it be removedLogPointIds?

Hmm, is it OK to keep the current capitalization? The nomenclature we've been using for this feature so far has treated 'logpoint' as a single word, as with 'breakpoint', e.g. see the title of bug 1526554. It would be nice to handle this consistently in the devtools to avoid confusion / typos.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Allow clearing console messages by their logpoint ID.

Updated patch per comments (though see comment 7).

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Hmm, is it OK to keep the current capitalization? The nomenclature we've been using for this feature so far has treated 'logpoint' as a single word, as with 'breakpoint', e.g. see the title of bug 1526554. It would be nice to handle this consistently in the devtools to avoid confusion / typos.

yeah sure. Now that you talk about breakpoint, I agree to keep that consistent.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9048304 [details] [diff] [review]
Part 2 - Allow clearing console messages by their logpoint ID.

Review of attachment 9048304 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me, thanks Brian!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0310f8849e0
Part 1 - Debugger changes for removing obsolete log points, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e997aba85bb
Part 2 - Allow clearing console messages by their logpoint ID, r=nchevobbe.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0385531495c5
Part 3 - Support conditional logpoints when replaying, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/033697893400
Part 4 - Add logpoint tests.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cccd561de71
Fix flow/lint failures.

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b0310f8849e0
https://hg.mozilla.org/mozilla-central/rev/1e997aba85bb
https://hg.mozilla.org/mozilla-central/rev/0385531495c5
https://hg.mozilla.org/mozilla-central/rev/033697893400
https://hg.mozilla.org/mozilla-central/rev/8cccd561de71