search

PostHog / posthog

🦔 PostHog provides open-source product analytics, session recording, feature flagging and A/B testing that you can self-host.
https://posthog.com
Other
21.62k stars 1.29k forks source link

Funnel time frame change not updating on the dashboard #5358

Closed liyiy closed 2 years ago

liyiy commented 3 years ago

Bug description

Please describe.
If this affects the front-end, screenshots would be of great help.

After updating a funnel's time frame from All time to 90 days, the insight itself updates but does not save to the dashboard correctly, rendering an incorrect old view

**note: I haven't been able to reproduce this error myself, I encounter a different 502 bad gateway error instead actually

Expected behavior

Updating a dashboard insight should render it correctly in the dashboards view.

How to reproduce

1. 2. 3.

Environment

Additional context

Thank you for your bug report – we love squashing them!

macobo commented 3 years ago

@paolodamico It'd be great if your team could take a look here - I think you should have enough context to own dashboards!

neilkakkar commented 3 years ago

I spent a while last night gathering context for this. More specifically, the issue seems to be that updating specific dashboard item date frames doesn't reflect on the dashboard, when the dashboard itself has a different date filter.

Easier to explain in code, so I wrote a test for it, but haven't yet managed to fix it.

It appears to be a feature request more than a bug: "Allow dashboard item time filters to be saved independently from dashboard time filters" - but when dashboard time filters are updated, update all item time filters as well.

Not enough context to decide the approach myself. The test, though, is here: https://github.com/PostHog/posthog/pull/5395

paolodamico commented 3 years ago

We're happy to take this on but based on @neilkakkar's work it sounds like you may already have a head start on this? Want us to pick it up?

When you talk about the context for the approach @neilkakkar do you mean UX-wise? As in how do global dashboard filters should override local filters? Just of the top of my head I think in general we should respect the most recent changes. I wonder if should introduce an option to let you switch between individually set filters and the global filter configured for the entire dashboard. Thoughts @clarkus ?

clarkus commented 3 years ago

Does the work happening at #3408 solve for this job? A user could click through the graph on the dashboard to the insight detail view to do any deeper analysis or explore different time ranges.

It appears to be a feature request more than a bug: "Allow dashboard item time filters to be saved independently from dashboard time filters" - but when dashboard time filters are updated, update all item time filters as well.

This could be really clever and powerful, but also really hard to communicate and really confusing for users. I would advocate for a simpler approach and default dashboards to fresh, recent time ranges (for example, the last 7 days, the last 24 hours, etc). The dashboard time context should be constant across all the graphs on the dashboard.

I could also be misunderstanding the problem we're trying to solve here. Is this a case where dashboards should also serve as tools for comparing unlike time ranges? I was seeing comparison ranges in insights as the solution for that, but there might be more to it? My biggest concern is that it's not obvious that a dashboard is reflecting multiple time ranges and a user draws conclusions thinking that all graphs share the same time range.

neilkakkar commented 3 years ago

I agree with @clarkus , it reflects some of my own thinking.

@paolodamico : I have so far just done the investigation to get to the core issue. About fixing / deciding not to do it, leaving it up to you?

More context on the use case: https://posthog.slack.com/archives/C01HZB27BGX/p1627898599000100?thread_ts=1627395111.001800&cid=C01HZB27BGX

paolodamico commented 3 years ago

UX-wise I'd like to propose this approach: user has the option to set a global dashboard time range or keep it in "Custom" / individual time ranges (in which the saved time range for insight is applied). Thing is that with #3408, dashboard items will now be accessible standalone and it'd be pretty confusing if their timeframe changes from a dashboard config (it further lends itself to more edge cases such as if a single insight is in multiple dashboards).

So if a dashboard has a global custom time range, that would prevail as the time range for all items, but only for that dashboard.

Thoughts?

macobo commented 3 years ago

That's exactly how it works currently AFAIK - we store dashboard-specific filters (e.g. time range) in the dashboard object and merge it with the items config. :)

clarkus commented 3 years ago

To make sure I understand how this works:

Is that accurate? Or is it that once an insight is on a dashboard it defers to the dashboard's time context?

paolodamico commented 3 years ago

Ok, so your scenario is correct @clarkus. However, there's the edge case where you have a dashboard with a configured global timeframe and afterwards you add a new insight. In this case, when you add it to the dashboard, it will also reflect the dashboard's global config.

So I see two UX issues with what we currently have today:

liyiy commented 3 years ago

Based on @neilkakkar's findings this seemed like an existing bug but apparently it's only started happening for users just recently?

clarkus commented 3 years ago

When you set a global time range in a dashboard it seems to affect the individual insight's timeframe when you open it from that dashboard (i.e. when you open it, it has the dashboard's timeframe as opposed to the individually set). As we introduce saved insights, this could cause confusion as the timeframe would change depending on how you're opening the insight.

Given the work happening in https://github.com/PostHog/posthog/issues/3408, I don't think we should change the time definition for an insight outside the context of directly editing that insight. If a dashboard can someone change the time definition saved for an insight, that's going to be really confusing.

The "Custom" label does not communicate well that it means you're using the individually configured timeframes. Further, it's not clear that if you've set a global timeframe you can go back to individual timeframes by setting this option again (which btw works perfectly 😁)

"Custom" feels like the wrong label here. I could see custom implying a specific, fixed range of time. What custom really means here is "defer to the time range of each insight". The dashboard has no time context. Maybe "none" would be more descriptive?

paolodamico commented 3 years ago

Given the work happening in #3408, I don't think we should change the time definition for an insight outside the context of directly editing that insight. If a dashboard can someone change the time definition saved for an insight, that's going to be really confusing.

100% agreed @clarkus, exactly my point as something to address.

Re "None", I don't think this communciates well what's going on either. Perhaps we should be more verbose here? Or at least combine with an info tooltip.

clarkus commented 3 years ago

I have been thinking about this and testing it a bit and I'm not sure custom actually works the way it's being described. In this video I have two charts - one set to the last 24 hours and one set to 90 days. Note that the dashboard reads this as custom. If I change this as at all to another option and then go back to "custom" the charts default to our 7 day range. This is the same default we use when starting a new insight. Is this how it's supposed to work?

https://user-images.githubusercontent.com/254612/128931287-b07d4a2c-2255-41dd-99dc-0487d95e3006.mov

I am working through some ideas for date control values that are more approachable.

clarkus commented 3 years ago

Frame 8712

Here's an idea that leans on the word "default" and some tooltips to explain what each option means. Here default is used for "defer to the insight time defined for each insight" and custom has been changed to "custom date range". Also, we have a ton of options in this list. Are all of these used?

paolodamico commented 3 years ago

Hmm definitely not the expected behavior. Did you try refreshing? Might be a frontend caching issue.

Really love the proposal for the dropdown and the tooltips, think definitely makes things a lot clearer. One additional thing worth considering is the role of timezone, particularly for short timeframes we may want to show something around that.

Great point on all the options, I believe we do capture information around all these options so will take a look tomorrow to see if we can optimize.

clarkus commented 3 years ago

Hmm definitely not the expected behavior. Did you try refreshing? Might be a frontend caching issue.

Yeah I tried it with cache disabled and it was reproducible.

Really love the proposal for the dropdown and the tooltips, think definitely makes things a lot clearer. One additional thing worth considering is the role of timezone, particularly for short timeframes we may want to show something around that.

I've been thinking about where to place the timezone conversion. We talked about placing in the user / utility menu updates, but maybe it needs to be more front and center. I had also tried in in the global header, but there was feedback that was too prominent. Ideally I'd like to place that prominently to set context for the entire product. Short of that, we can append it to the control that's setting the time context for everything below it.

Screen Shot 2021-08-10 at 8 15 43 PM
paolodamico commented 3 years ago

I think having it here makes a lot of sense. It's clear and right where you need it.

mariusandra commented 3 years ago

I fixed one bug reported here. Whenever we would have a custom date range in the filter without an end time (e.g. "2021-05-10" - today), the filter would say "Last 7 days". Now it'll give the right number of days. Why the date was hardcoded in the range to begin with, I don't know.

Regarding dashboard items with custom date ranges, the proposed solution makes sense. However if we're to be accurate, I think we should communicate the per-insight date range within the dashboard item. Or will custom descriptions ("Browsers last 30 days") and chart axes be enough? I'm leaning towards "explicit is better than implicit". We could show the same "Last 7 days" dropdown inside or above each dashboard item, even if just in a tooltip with a calendar icon.

One added consideration. Anyone can change the date filter on a dashboard and it will then persist the new value in the "database items" table for all insights on the dashboard, overriding whatever was there before. This means after changing the date filter once, we won't have "default" value to revert back to. What to do? Should we display a warning/alert when changing away from the "default" option to something else? Should we update the database schema to persist the default range for each insight and have some flakey code to swap them back?

clarkus commented 3 years ago

Regarding dashboard items with custom date ranges, the proposed solution makes sense. However if we're to be accurate, I think we should communicate the per-insight date range within the dashboard item. Or will custom descriptions ("Browsers last 30 days") and chart axes be enough? I'm leaning towards "explicit is better than implicit". We could show the same "Last 7 days" dropdown inside or above each dashboard item, even if just in a tooltip with a calendar icon.

I don't think we can rely on meaningful names or descriptions. I agree that we should be explicit here. I'm not sure about allowing for time context changes at a granular level like this. I mean it's very flexible, but it could also lead to a dashboard state that is very confusing. Also if the time context changes and it disagrees with any representation of time in the dashboard item name, it could lead to more confusion.

One added consideration. Anyone can change the date filter on a dashboard and it will then persist the new value in the "database items" table for all insights on the dashboard, overriding whatever was there before. This means after changing the date filter once, we won't have "default" value to revert back to. What to do? Should we display a warning/alert when changing away from the "default" option to something else? Should we update the database schema to persist the default range for each insight and have some flakey code to swap them back?

I encountered this previously and it is very confusing. How does the work happening for https://github.com/PostHog/posthog/issues/3408 impact this behavior? Are we modifying the time context for a "saved insight" outside of the editing / creation workflow? Does this only impact the previous model for saving insights? I feel like it's going to be a pretty bad experience if something other than the insight editing workflow modifies your insight definition.

There are other considerations, too. With funnels, the time to convert option should be kept in sync with the time range for the the visualization. If those can be changed independently without any synchronization, it could lead to really confusing results. https://github.com/PostHog/posthog/issues/5557

paolodamico commented 3 years ago

Updating here on the date options based on usage in the last 90 days. Based on the data, I think we can get rid of the following options due to low usage (less than 1% of insights) and to simplify:

CC @clarkus

clarkus commented 3 years ago

👍 on dropping the options that are seeing less use.

Frame 8712 https://www.figma.com/file/9yWtngNb1AIuf6KmXaEPJA/App-doodles?node-id=986%3A136255

paolodamico commented 2 years ago

Superseded by #6712

posthog-contributions-bot[bot] commented 2 years ago

This issue has 2282 words at 22 comments. Issues this long are hard to read or contribute to, and tend to take very long to reach a conclusion. Instead, why not:

  1. Write some code and submit a pull request! Code wins arguments
  2. Have a sync meeting to reach a conclusion
  3. Create a Request for Comments and submit a PR with it to the meta repo or product internal repo

Is this issue intended to be sprawling? Consider adding label epic or sprint to indicate this.