Skip to content

feat(nimbus): add analaysis window indicator inside metric name cards#15239

Open
moibra05 wants to merge 1 commit intomainfrom
14850
Open

feat(nimbus): add analaysis window indicator inside metric name cards#15239
moibra05 wants to merge 1 commit intomainfrom
14850

Conversation

@moibra05
Copy link
Copy Markdown
Contributor

Because

  • It's often times unclear which analysis window is currently being displayed for a metric's results

This commit

  • Adds a text indicator that tells us which analysis window is being reflected in the main page results

Fixes #14850

@moibra05
Copy link
Copy Markdown
Contributor Author

image

Comment on lines +390 to +393
if kpi["slug"] == NimbusConstants.RETENTION_3_DAYS:
kpi["displayed_window"] = "Day 4"
if kpi["slug"] == NimbusConstants.RETENTION:
kpi["displayed_window"] = "Week 2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these always accurate based on the data available? Like what happens on Day 3? Do we show "Day 4" for RETENTION_3_DAYS, or do we just show nothing? And I'm fairly sure Week 2 for Retention could be wrong in some cases because we haven't done #12647 yet, so it could be Week 1 (the old Results page always said Week 2 Retention so maybe we don't need to block on this).

I just want to make sure we aren't going to be potentially showing the wrong thing for early or wonky (missing days/weeks) results.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair points. the changes im making in #15050 will make it so 3day retention will remain unavailable until day 4 data is available so it will show nothing on Day <4 or if the metric has no data (this goes for all metrics). in that case i think the most logical thing to do would be to complete #15050 and #12647 before this

@mikewilli
Copy link
Copy Markdown
Contributor

There was some recent confusion on what the Overview section of the metric details modal is showing -- can/should we tack something on to this PR to update that too? Or file a new ticket?

@moibra05
Copy link
Copy Markdown
Contributor Author

i think we can just include that change in this pr and broaden the scope/title to covering general improvements to analysis window representation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display which time window each metric is using on results

2 participants