Closed Bug 1531395 Opened 10 months ago Closed 8 months ago

Add telemetry to track persist option usage in the Console panel

Categories

(DevTools :: Console, enhancement, P3)

enhancement

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: Honza, Assigned: lloanalas, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: good-first-bug)

Attachments

(2 files, 1 obsolete file)

We should have some stats about how many users are using the "Persist Log" option.

Honza

Bug 1463095 might be a source of inspiration.

@Nicolas: any other bugs we could refer to to make the implementation easier?

Honza

Flags: needinfo?(nchevobbe)
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: good-first-bug

Lucklily, we already have a mechanism to handle telemetry calls.

So, in the console, in order to change the "Persist Logs" option, we use a Redux action, persistToggle, defined in devtools/client/webconsole/actions/ui.js#24-32.
We can see that we dispatch an object with a PERSIST_TOGGLE type.

Then, we have a telemetry middleware, i.e. a piece of code that will be triggered on each state change (so, it will be called when the PERSIST_TOGGLE action is dispatched as well).

The middleware is in devtools/client/webconsole/middleware/event-telemetry.js

This is the main function:

/**
 * Event telemetry middleware is responsible for logging specific events to telemetry.
 */
function eventTelemetryMiddleware(telemetry, sessionId, store) {
  return next => action => {
    const oldState = store.getState();
    const res = next(action);
    if (!sessionId) {
      return res;
    }

    const state = store.getState();

    const filterChangeActions = [
      FILTER_TEXT_SET,
      FILTER_TOGGLE,
      DEFAULT_FILTERS_RESET,
    ];

    if (filterChangeActions.includes(action.type)) {
      filterChange({
        action,
        state,
        oldState,
        telemetry,
        sessionId,
      });
    } else if (action.type === MESSAGES_ADD) {
      messagesAdd({ action, telemetry });
    }

    return res;
  };
}

You can see that we already have some function calls that are done when a given action is dispatched.
So for this bug, we will have another else if where we check the PERSIST_TOGGLE action type.

For this case, we will use the telemetry object to record the event, like:

  telemetry.recordEvent("persist_changed", "webconsole", null, {
    "active": /** herer we need to retrieve the value of the persist checkbox **/,
    "session_id": sessionId,
  });
Mentor: nchevobbe
Flags: needinfo?(nchevobbe)

Hey,
I nobody is working on it, I would like to work on this.
Can you please assign it to me?

Hi, Outreachy applicant here. I'm also interested in working on this issue, just in case previous requester already has bugs assigned.

Thanks a ton!

Flags: needinfo?(odvarko)

hello Paarmita, lloan,
Since Paarmita was first to ask, I'm going to assign her to bug.
lloan, it would be nice if you could also look at the bug, and assist Paarmita here. I'm thinking this might need a test to make sure we do listen for those events, so you could do some research on how to add such tests, and share your findings here :) is that okay for you?

Assignee: nobody → paarmita1998
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)

(In reply to Nicolas Chevobbe from comment #5)

hello Paarmita, lloan,
Since Paarmita was first to ask, I'm going to assign her to bug.
lloan, it would be nice if you could also look at the bug, and assist Paarmita here. I'm thinking this might need a test to make sure we do listen for those events, so you could do some research on how to add such tests, and share your findings here :) is that okay for you?

Sure thing boss, sounds good!

(In reply to Paarmita Bhargava from comment #3)

Hey,
I nobody is working on it, I would like to work on this.
Can you please assign it to me?

In devtools/client/webconsole/middleware/event-telemetry.js line #43 I added this:

    else if (action.type === PERSIST_TOGGLE) {
      telemetry.recordEvent("persist_changed", "webconsole", null, {
        "active": action.value.toString(),
        "session_id": sessionId,
      }); 

Also, in line 7, I added PERSIST_TOGGLE as well.

Then in event-telemetry.js I updated the persistToggle function to this:

function persistToggle() {
  return ({dispatch, getState, prefsService}) => {
    const uiState = getAllUi(getState());
    dispatch({
      type: PERSIST_TOGGLE,
      value: uiState.persistLogs
    });    
    prefsService.setBoolPref(PREFS.UI.PERSIST, uiState.persistLogs);
  };
};  

Only issue I'm facing at the moment is that when I toggle the persist log state, its giving me this error:

Error: Unknown event: ["devtools.main", "persist_changed", "webconsole"] in telemetry.js:571:24

I reached out to Honza to see if I had to register the event prior to using it. Have you had any luck with your approach - what have you tried?

:nchevobbe I tried searching some more about this online, but came up pretty short in terms of documentation. My question is, do I need to register the event 'persist_changed' - I'm thinking that might be my issue. Trying to figure out if the code base already has a list of allowed events to add to it, but can't seem to find something yet, figured I'd reach out to someone just in case they knew off the top of their head.

Flags: needinfo?(nchevobbe)

Paarmina,

I was able to add the event in /toolkit/components/telemetry/Events.yaml under devtools.main:

persist_changed:
    objects: ["netmonitor", "webconsole"]
    bug_numbers: [1531395]
    notification_emails: ["dev-developer-tools@lists.mozilla.org", "hkirschner@mozilla.com"]
    record_in_processes: ["main"]
    description: User has changed log persist status.
    release_channel_collection: opt-out
    expiry_version: never
    extra_keys:
      persist_state: "The log persist status."
      session_id: The start time of the session in milliseconds since epoch (Unix Timestamp) e.g. 1396381378123.

I think :nchevobbe could step in on this part and tell us if that's correctly formatted.

Also updated the else if section to reflect the use for the data being passed in as extra values to the event

devtools/client/webconsole/middleware/event-telemetry.js

} else if (action.type === PERSIST_TOGGLE) {
      telemetry.recordEvent("persist_changed", "webconsole", null, {
        "persist_state": action.status.toString(),
        "session_id": sessionId,
      }); 
    }

Also updated the persistToggle function for the same reason:

function persistToggle() {
  return ({dispatch, getState, prefsService}) => {
    const uiState = getAllUi(getState());
    dispatch({
      type: PERSIST_TOGGLE,
      status: uiState.persistLogs
    });    
    prefsService.setBoolPref(PREFS.UI.PERSIST, uiState.persistLogs);
  };
};  

Don't forget to add PERSIST_TOGGLE here as well: devtools/client/webconsole/middleware/event-telemetry.js

const {
  FILTER_TEXT_SET,
  FILTER_TOGGLE,
  DEFAULT_FILTERS_RESET,
  MESSAGES_ADD,
  PERSIST_TOGGLE,
} = require("devtools/client/webconsole/constants");

:nchevobbe - do we need to write a test for this?

Flags: needinfo?(nchevobbe) → needinfo?(paarmita1998)

Yes, a test would be nice.
We already have a test for the "filters_changed" event here: devtools/client/webconsole/test/mochitest/browser_webconsole_telemetry_filters_changed.js.

(In reply to Nicolas Chevobbe from comment #9)

Yes, a test would be nice.
We already have a test for the "filters_changed" event here: devtools/client/webconsole/test/mochitest/browser_webconsole_telemetry_filters_changed.js.

Awesome! I'll let :Paarmita handle the test and see if she needs any help there.

Flags: needinfo?(paarmita1998)

:paarmita How is it going with the test? Anything else you want to bounce off me?

Flags: needinfo?(paarmita1998)

Bug 1531395 - Add telemetry to track persist option usage in the Console panel

Flags: needinfo?(paarmita1998)

Paarmita(In reply to Paarmita Bhargava from comment #12)

Created attachment 9052184 [details]
Bug 1531395 - Add telemetry to track persist option usage in the Console panel

Bug 1531395 - Add telemetry to track persist option usage in the Console panel

Paarmita, I haven't been able to reach you on Slack for a few days now, but take a look here -> https://phabricator.services.mozilla.com/D26046 - test this out, let me know if you have any questions or if you have any improvements.

Flags: needinfo?(paarmita1998)
Flags: needinfo?(nchevobbe)

Paarmita, I'm assigning the bug to lloan since his patch is almost ready to land, and yours still suffers from eslint failures (also I think you have other assigned bug).

Assignee: paarmita1998 → lloanalas
Flags: needinfo?(paarmita1998)
Flags: needinfo?(nchevobbe)
Attached file data-review.txt
Attachment #9057444 - Flags: data-review?(francois)
Attachment #9052184 - Attachment is obsolete: true
Comment on attachment 9057444 [details]
data-review.txt

Hi Nicolas, I left Mozilla last year and so I'm not a data steward anymore. You'll have to pick someone from the list at the top of https://wiki.mozilla.org/Firefox/Data_Collection.
Attachment #9057444 - Flags: data-review?(francois)
Attachment #9057444 - Flags: data-review?(chutten)

Thanks for letting me know François!

Comment on attachment 9057444 [details]
data-review.txt

Preliminary note:

In your answer to Q3 you say you already have some collections in this area that aren't getting the job done. Are these scheduled for removal since this collection will take their place?

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Harald Kirschner is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+
Flags: needinfo?(nchevobbe)
Attachment #9057444 - Flags: data-review?(chutten) → data-review+

In your answer to Q3 you say you already have some collections in this area that aren't getting the job done. Are these scheduled for removal since this collection will take their place?

Sorry Chris, this is erroneous (copied from another data review 😕), we don't have anything right now for this behavior.
Should I update the document?

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c503fc359408
Add telemetry to track persist option usage in the Console panel. r=nchevobbe

(In reply to Nicolas Chevobbe [:nchevobbe] [on PTO 04/17 -> 04/23 ] from comment #20)

In your answer to Q3 you say you already have some collections in this area that aren't getting the job done. Are these scheduled for removal since this collection will take their place?

Sorry Chris, this is erroneous (copied from another data review 😕), we don't have anything right now for this behavior.
Should I update the document?

This note here is documentation enough, no worries. I'm just trying to keep our data collections clean : )

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.