Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify relationship between insights, dashboards and date filters #6712

Closed
mariusandra opened this issue Oct 28, 2021 · 12 comments
Closed

Clarify relationship between insights, dashboards and date filters #6712

mariusandra opened this issue Oct 28, 2021 · 12 comments

Comments

@mariusandra
Copy link
Collaborator

mariusandra commented Oct 28, 2021

We talk about "creating new insights" and then either "saving" them or "adding them to a dashboard".

It feels natural that you could add an insight onto multiple dashboards. You painstakingly create a conversion funnel, and would like to see it on both the sales and marketing dashboards. A pretty common usecase I'd think.

That's unfortunate, because this is currently impossible. An "insight" is actually a "dashboard item" in the database. It's tightly coupled to the dashboard it's on, and looks something like this:

export interface DashboardItemType {
    id: number
    name: string
    short_id: string
    description?: string
    favorited?: boolean
    filters: Partial<FilterType> // <-- filters on the insight, match the cached `result` below
    filters_hash: string
    order: number
    deleted: boolean
    saved: boolean
    created_at: string
    layouts: Record<string, any> // <-- where on the dashboard this insight is
    color: string | null // <-- what color is it?
    last_refresh: string
    refreshing: boolean
    created_by: UserBasicType | null
    is_sample: boolean
    dashboard: number | null // <-- dashboard_id
    dive_dashboard?: number // <-- why do we have two of them? to add it an insight to more than one dashboard? 🤔
    result: any | null // <-- cached results for the "filters" or nothing
    updated_at: string
    tags: string[]
    next?: string // only used in the frontend to store the next breakdown url
}

If we want to make it possible to save an insight onto multiple dashboards, we need to add a table between insights and dashboards to create a has-many relationship. The question is: what data should remain on the insight, and what data should remain on the link between the dashboard and the insight?

The answer is more complicated than it seems.

More practically:

  • If you change the name of a dashboard item, will it change the name on all dashboard items and also the insight, or only on that dashboard? Or we ask the user what they want?
  • We currently have actions "copy to dashboard" and "move to dashboard" on dashboard items. How do we handle these?
  • The layouts

However these are the easy questions. There's a worse problem: when you update the global date filter on a dashboard, it updates and overrides in the database, the filters (with the new date), and results on each of the insights that are shown.

If we'd just share insights between dashboards now, we'll end up with some insights constantly overriding themselves, if they're on dashboards with different global date filters. Not cool.

Hence we need to rethink the relationship an insight has with its date filters, and how that gets persisted. If multiple dashboards share an insight, and use different date filters, then the cached result on the object is also quite worthless, as it'll be for a different set of filters.

Hence my proposal:

// create a new table "insight", migrate over old dashboard items
// no new fields added, just moved them over
export interface InsightType {
    id: number
    name: string
    short_id: string
    description?: string
    tags: string[]
    favorited?: boolean
    filters: Partial<FilterType> // <-- filters on the insight, match the cached `result` below
    filters_hash: string
    deleted: boolean
    saved: boolean
    created_at: string
    created_by: UserBasicType | null
    updated_at: string

    last_refresh: string
    refreshing: boolean
    result: any | null // <-- cached results for the default "filters", just for the "saved insights" view
}

// either create a new DashboardInsight table as well, or keep the old DashboardItem table, 
// but just deprecate a lot of fields and add an insight_id ... not sure if a clean slate is better or not
export interface DashboardItemType {
    dashboard_id: number
    insight_id: number 
    created_at: string
    layouts: Record<string, any> // where on the dashboard this insight is
    color: string | null 
    is_sample: boolean
    order: number // is this needed? copied from dashboard item
    filter_override: Partial<FilterType>
 }

The filter_override field (or custom_date_filter?) in the dashboard item model would contain filters that are different than filters on the insight. Just the date filter for now, but perhaps others eventually? This would then allow to build a feature, which lets power users override date filters on specific dashboard items. This has been requested repeatedly.

Also, I'm not sure if we should keep the cached results on the insight type at all, or if we should just rely on a redis caching layer for this? Should we also add a result on the dashboard item to cache it with the applied set of date filters? We could also instead build a postgres-based InsightCache table that'd unify filtering across filters_hash.

This task is quite complex (many moving parts) and will probably fall outside of releasing saved insights, just so we don't need to fix all these edge cases: so not this week, but it could be part of the scope for next sprint.

Many questions. Any feedback on the proposed split? @Twixes @paolodamico @clarkus

Related epic: #6513
Related issue: #6559

@clarkus
Copy link
Contributor

clarkus commented Oct 28, 2021

The filter_override field (or custom_date_filter?) in the dashboard item model would contain filters that are different than filters on the insight. Just the date filter for now, but perhaps others eventually? This would then allow to build a feature, which lets power users override date filters on specific dashboard items. This has been requested repeatedly.

I would also consider things closely related to dates / time ranges:

  • Time units - we would want to enable a user to adjust this to better align with the time range
  • Comparison ranges - not applicable to every visualization type, but definitely very related to time ranges, units, etc.
  • Simple layout toggles - if it's just changing from a vertical to horizontal layout, we should allow for that.
  • Visualization types - unless it's closely tied to the insight type, then allow a user to change viz types might make sense in some cases.

Generally anything that doesn't change the underlying data definition, but modifies how you might organize or parse the data seems like it should be available. Thank you for documenting this and initiating a conversation on how to resolve it. I think it's a very critical issue.

@paolodamico
Copy link
Contributor

The proposed split makes sense to me. Some points.

  • The short_id is used (or will be fully used) to generate permalinks, we may want to share permalinks in the scope of the dashboard location, so we might consider moving this to DashboardItemType (or add an extra param in the permalink which actually think is a better idea).
  • As part of this I would also consider permanently deleting items instead of just soft deleting. Alternatively, we could have a cron job that permanently deletes stuff that has been soft deleted more than X weeks ago.
  • I generally agree on @clarkus point ("Generally anything that doesn't change the underlying data definition, but modifies how you might organize or parse the data seems like it should be available") ,but I wonder if it can introduce some confusion, for instance if we should expose changing the visualization type (bar/area/line...).

@clarkus
Copy link
Contributor

clarkus commented Oct 28, 2021

for instance if we should expose changing the visualization type (bar/area/line...).

I could go either way on this. I'm fine leaving it out of scope if it makes decisions easier for now. A user could easily duplicate and modify the insight if they really wanted an alternate viz type.

@mariusandra
Copy link
Collaborator Author

One extra consideration. We will surely also want to add things like correlation analysis results, session recordings, random tables from random scenes, etc onto a dashboards. Currently you can only add things that are "insights", that means the main graph for any tab under the "insights" menu item.

This could be possible, and will as its first step require a split between insights and dashboard items.

@clarkus
Copy link
Contributor

clarkus commented Oct 29, 2021

I think we might consider a generic "Add to dashboard" feature for any visualization that is the result of an insight / analysis. Dashboards might become more complex depending on the scope of that action. For example, dashboards could be a collection of a random snapshot of independent items, or they could be a related set of things with a shared context (time for example).

@Twixes
Copy link
Member

Twixes commented Nov 1, 2021

+1 to the proposal, with Paolo's suggestion. My thoughts:

  • I do think it'd be more flexible to cache via Redis instead.
  • I'd prefer a clean, simpler slate for new dashboard items (will need a different table name though)…
  • Considering custom time ranges, dashboard items may need to have their own last_refresh/refreshing.
  • I think we can drop is_sample, though @paolodamico may have more context if that's still useful.

@paolodamico
Copy link
Contributor

paolodamico commented Nov 1, 2021

We can drop is_sample for now (but let's delete the corresponding sample dashboard items too). Its original intent was to track down dashboard items that were automatically created by us as a way to onboard users, but it's not really getting a lot of usage, nor is it driving activation now. CC @kpthatsme

@mariusandra
Copy link
Collaborator Author

This task is evolving in scope, and I think it makes sense to dedicate someone working on this for a full sprint later on.

Some new context which should be addressed if we redo the dashboard backend:

  • We want to put other stuff on dashboards, not just insights. For example correlation analysis. Or recordings. Or a list of persons belonging to a cohort. Or the list of persons dropped off at a certain step in a funnel in the last week.
  • Currently the entire dashboard item system is built around showing insights. We have one main insightLogic that contains all the code to reload the insight, and also code to show a cached insight without making a request to load results (cachedResults).
  • We have since developed some features (e.g. correlation analysis) that are tied to an insight, but are fetched as a separate query after the insight itself has loaded. This used to be the case for the funnel time to convert view, but we fixed that.
  • Shared dashboards complicate matters. On them, you're not authorised to make random requests via the dashboard share token. You can just talk to the dashboard and ask it for data, but you can't change any filters.
  • How could we change things to add correlation analysis to the dashboard? It's not an insight...?
  • Same with e.g. recordings, person lists, etc.

Instead of a bespoke integration with each different thing we can add to a dashboard, could there be a good pattern we can implement to be able to add anything on a dashboard? Both for the backend and the frontend.

@macobo
Copy link
Contributor

macobo commented Nov 8, 2021

My 2c:

  • (!) Let's make sure all usecases we're building for are covered by real user use-cases and document each.
    • E.g. I'd go deep to understand the need for "person modal in dashboard" and "session recordings" since they sound like we might not understand the proper problem being solved.
    • Cost of getting this wrong is huge as killing misfeatures is larger than not having them.

Below I'll assume there's a legit use-case:

  • Definitions are key. I'd define an "insight" as a "time-series metrics query result".
    • Note that this covers everything here except session recordings - I think session recordings need fundamentally to have a different system from other insights. See below how I'd fit it into the architecture though
  • Insights should be uniquely defined by the "filter" object in both the FE and backend. This includes second-order queries like correlation analysis.
    • Only given a filter and a result set it should always be possible to both uniquely query + render results.
  • Rather than have N different endpoints for each, we should have a single endpoint to accept the filter object that does multiple dispatch down a chain.
    • In frontend this kind of has already materialized with the insightsLogic, in the backend kind of with the cache refreshes stuff, all excepting
    • This should significantly simplify e.g. dashboard items refreshing logic as well.
  • Schema-wise:
    • Fix Updating dashboard date filter should not update all dashboard items filters #6936 - dashboard level filters should not touch items
    • Make dashboard items to dashboards many-to-many, store layout information on the edges.
    • When adding support for session recordings, add type field to DashboardItem. Can be session_recordings if querying session recordings, otherwise insights. This will control the semantics of the filter object.
    • Other details are discussed in-depth above :)

@clarkus
Copy link
Contributor

clarkus commented Dec 14, 2021

I have some work in progress on this issue that is ready at https://www.figma.com/file/gQBj9YnNgD8YW4nBwCVLZf?node-id=5958:45941#133212611. The core change is to always represent the insight's time context in the card. Insights can have varying time contexts, and that's OK!, but it's the job of each insight card to communicate this. Secondarily, the global date control will have an updated appearance to reinforce when no global time context is set. This is further reinforce by insight cards - when a global time context is set, the insight cards will report that same range (if applicable to the visualization parameters). This is an early idea, so please feedback in figma if you have any.

Screen Shot 2021-12-14 at 10 28 43 AM

Screen Shot 2021-12-14 at 10 28 35 AM

@paolodamico
Copy link
Contributor

Love this! Perhaps add an icon with a detail tooltip when the time range comes from a global config ?

@Twixes
Copy link
Member

Twixes commented Aug 2, 2023

This is solved now.

@Twixes Twixes closed this as completed Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants