Update messages shown according to the currently enabled logpoints

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 2 bugs)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Posted patch patchSplinter Review

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.

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

Attachment #9046175 - Flags: review?(lsmyth)

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.

Attachment #9046176 - Flags: review?(nchevobbe)

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).

Attachment #9046177 - Flags: review?(lsmyth)

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.

Attachment #9046177 - Flags: review?(lsmyth) → review+
Attachment #9046175 - Flags: review?(lsmyth) → review+
Blocks: 1531381
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`?
Attachment #9046176 - Flags: review?(nchevobbe)

(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.

(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.

Updated patch per comments (though see comment 7).

Attachment #9046176 - Attachment is obsolete: true
Attachment #9048304 - Flags: superreview?(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 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!
Attachment #9048304 - Flags: superreview?(nchevobbe) → superreview+
Pushed by bhackett@mozilla.com:
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.
Depends on: 1533260
You need to log in before you can comment on or make changes to this bug.