[GitHub] williaster opened a new pull request #5087: [dashboard v2] logging updates

[GitBox <gi...@apache.org>]

williaster opened a new pull request #5087: [dashboard v2] logging updates
URL: https://github.com/apache/incubator-superset/pull/5087
 
 
   This PR updates Dashboard logging for dashboard v2. It 
   1) captures additional client events we want to analyze and 
   2) updates how we capture performance, which is now more complicated because of `Tabs` and nested `Tabs`. 
   
   Some events are logged immediately, while others are "batched" similar to the current dashboard logging.
   
   #### Event name changes (made for improved event name clarity)
   - `page_load_perf` => `load_dashboard_pane` (see below)
   - `load_events` => `load_chart_data`
       - this event now includes `vis_type` and `has_extra_filters` fields
   - `render_events` => `render_chart` 
       - note: this event already included `vis_type`
   
   #### New click events that are captured   
   - `force refresh chart`
   - `force_refresh_dashboard`
   - `change_dashboard_filter`
   - `explore_dashboard_chart`*
   - `export_csv_dashboard_chart`*
     
   *denotes dispatched immediately, the others cause page re-renders and result in a `load_dashboard_pane` batch event 
   
   #### Updated performance logs
   Previously we captured a single `page_load_perf` event the first time all dashboard charts were loaded, and batched all chart load + render events with this event. This event 
   - does not capture subsequent page loads (e.g., when a filter is added, or an entire dashboard is refreshed) 
   - no longer makes sense in the context of dashboard v2 tabs + nested tabs
   
   This PR replaces the single `page_load_perf` event with (potentially multiple) `load_dashboard_pane` events, each of which is a batched event. A dashboard "pane" is any of the following:  
   - simply the dashboard page if no tabs exist
   - A top-level tab
   - A dashboard-level tab (a tab within the page)
   
   A `load_dashboard_pane` event is dispatched *every time* all of the charts within a pane (not including any nested within another set of tabs!) go from an `unloaded => loaded` state. This includes when a filter changes or force refreshes. Every `load_dashboard_pane` event includes the following metadata about the pane:
   - `start_offset` ms offset from page load time when the pane began to load (if you click on a tab 2 minutes into a session, this would be (`2 min * 60 s/min * 1000 min/s`) 
   - `duration` ms duration of all chart data requests
   - `id` the layout component id of the pane
   - `type` the layout type of the pane (grid or tab)
   - `parentType` parent pane type, `null` if the pane is a top-level tab or no top-level tabs exist
   - `parentId` id of the parent layout component (useful if you want to associate two tab panes with a common set of tabs)
   - `index` the index of the pane (0 for grid, 0-5 for tabs)
   - `depth` the depth of the pane (`0` for grid or top-level tabs, `1` for nested tabs [you cannot nest tabs beyond this])
   - `chartCount` number of charts within the tab (not including any nested tabs)
   
   ### New performance logging
   - A new `mount_dashboard` event was added and will be included in the first `load_dashboard_pane` batch event. The `start_offset` value in this event is effectively the amount of time it takes for a user to see something on their screen (so, bundle download + parsing time) and marks the point at which the Dashboard React component mounts and therefore chart queries begin.
   
   ### General `logger.js` changes
   I made the following changes/improvements to the logger util file
   - do not allow multiple `ActionLogger`s to handle the same event type (I'm not convinced this wasn't buggy previously, and seems unnecessary)
   - add `event_name` and `impression_id` to all `events` included in a batch action (to support unrolling batched events into multiple rows and re-associating with the batch action)
   - batched log `source` value changed from `client` to `dashboard` or `slice` depending on its source
   - removes top-level `duration` number from batched logs, this makes less sense now that duration comes from individual pane loads and is in the `events` themselves
   - removes top-level `actionType` from batched logs, it wasn't clear to me why this was needed if individual events have names
   
   @graceguo-supercat let me know if you see any issues with this + the changes I made to `logger.js` 
   @john-bodley @michellethomas  @mistercrunch 
   
   ![image](https://user-images.githubusercontent.com/4496521/40569402-2fae20e0-6036-11e8-97b2-76dcc18ccf0c.png)
   
   **Remaining TODO**
   - [ ] test / validate 
     - [ ] batching
     - [ ] multiple types of tab layouts
     - [ ] explore view
   - [ ] filter render events that come from chart resizes?
   - [ ] hook up refresh chart + refresh dashboard events
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org