feat(analytics): add monthly active users in highlights #7341
Conversation
...ore/src/main/java/com/linkedin/datahub/graphql/analytics/resolver/GetHighlightsResolver.java
Outdated
Show resolved
Hide resolved
| "Weekly Active Users", | ||
| "%.2f%% %s from last week", | ||
| endDate, | ||
| (date) -> date.minusWeeks(1) |
There was a problem hiding this comment.
Why is this a function? It seems more readable to me to just directly put the startDate here
There was a problem hiding this comment.
Because the lambda is re-used multiple times.
| DateTime endDate = DateTime.now(); | ||
| highlights.add(getTimeBasedHighlight( | ||
| "Weekly Active Users", | ||
| "%.2f%% %s from last week", |
There was a problem hiding this comment.
Can we make this change string a top level constant?
There was a problem hiding this comment.
This is unique to this chart and won't be reused.
|
|
||
| DateTime endDate = DateTime.now(); | ||
| highlights.add(getTimeBasedHighlight( | ||
| "Weekly Active Users", |
There was a problem hiding this comment.
Can we also make these constants?
There was a problem hiding this comment.
This is unique to this chart and won't be reused.
|
Marking as draft as some more changes need to be done which will require more testing. Will mark as ready for review once those changes are done. |
| @@ -42,39 +42,17 @@ function computeLines(chartData: TimeSeriesChartType, insertBlankPoints: boolean | |||
|
|
|||
| const startDate = new Date(Number(chartData.dateRange.start)); | |||
| const endDate = new Date(Number(chartData.dateRange.end)); | |||
| const intervalMs = INTERVAL_TO_SECONDS[chartData.interval] * 1000; | |||
There was a problem hiding this comment.
this was causing problem with Months related maths.
| final DateInterval interval, | ||
| final Function<DateTime, DateTime> convertToBegin | ||
| ) { | ||
| final DateTime beginning = convertToBegin.apply(endDateTime); |
There was a problem hiding this comment.
minor naming nit. you can call this beginningDateTime to keep consistency
| .withMinuteOfHour(0) | ||
| .withSecondOfMinute(0) | ||
| .withMillisOfDay(0) | ||
| )); |
| Double percentChange = | ||
| (Double.valueOf(weeklyActiveUsers) - Double.valueOf(weeklyActiveUsersLastWeek)) / Double.valueOf( | ||
| weeklyActiveUsersLastWeek) * 100; | ||
| if (activeUsersLastRange > 0) { |
...ore/src/main/java/com/linkedin/datahub/graphql/analytics/resolver/GetHighlightsResolver.java
Show resolved
Hide resolved
...ore/src/main/java/com/linkedin/datahub/graphql/analytics/resolver/GetHighlightsResolver.java
Show resolved
Hide resolved
datahub-web-react/src/app/analyticsDashboard/components/TimeSeriesChart.tsx
Show resolved
Hide resolved
| const returnLines: NamedLine[] = []; | ||
| chartData.lines.forEach((line) => { | ||
| const newLine = [...line.data]; | ||
| for (let i = endDate.getTime(); i > startDate.getTime(); i -= intervalMs) { | ||
| for (let i = startDate; i <= endDate; i = addInterval(1, i, chartData.interval)) { |
There was a problem hiding this comment.
Nice use of add interval!
Checklist