Move Top Stats to new internal stats endpoint#6035
Move Top Stats to new internal stats endpoint#6035RobertJoonas wants to merge 12 commits intomasterfrom
Conversation
|
329200e to
1fcedf4
Compare
6732692 to
6c0a88d
Compare
6c0a88d to
eb2b22c
Compare
| with {:ok, compare} <- parse_include_compare(params["include"]) do | ||
| {:ok, | ||
| %QueryInclude{ | ||
| imports: params["include"]["imports"] !== false, |
There was a problem hiding this comment.
Strict equality check only matters when comparing integers against floats (https://hexdocs.pm/elixir/1.19.5/Kernel.html#===/2). Using ==/!= should be fine enough.
Besides, this will evaluate to true when params["include"]["imports"] evaluates to nil, for instance. Is that intended?
There was a problem hiding this comment.
Using ==/!= should be fine enough
🤦🏻♂️ Always happens to me when working on JS and Elixir in parallel. Fixed in 19f3bf9.
Also, good shout about evaluating to true on invalid/missing values. When we first built the Dashboard.QueryParser (with a LiveView dashboard in mind) there was a requirement to provide defaults matching the current dashboard behaviour.
However now that there's a StatsQuery on the React end, I think it makes sense for that StatsQuery to define default values instead. (And make Dashboard.QueryParser dumb about defaults). I did that in the same commit (19f3bf9)
| pagination: query.pagination | ||
| ) | ||
| meta: meta(runner) |> Jason.OrderedObject.new(), | ||
| query: query(runner) |> Jason.OrderedObject.new() |
There was a problem hiding this comment.
More of a side note - but perhaps I should have pulled the trigger on removal of these clunky serializations in #5994. 🙈 They seem to be purely for aesthetics in docs playground.
There was a problem hiding this comment.
Yeah, it's a bit annoying for sure, but I'm not that bothered by it. But if we can find a way to get rid of it, all fine by me! 😄
Changes
This PR introduces a new
POST /api/stats/:domain/queryendpoint and makes the Top Stats use it. The endpoint will eventually replace all the Api.StatsController actions such asmain_graph,sources,pages, etc.statsfunction inapi.ts. It takes aStatsQueryobject which aims to mimic the query params required to construct%ParsedQueryParams{}on the backend.StatsQueryobject, there's also a new function calledcreateStatsQuerywhich takesDashboardStateandReportParamsobjects as arguments. ReportParams contains the information about a specific report, e.g.: dimensions and metrics.While this PR does not intend to change any behaviour, there are some functional changes with this move:
bounce_rateis now shown in page-filtered Top Stats, even when imports are included in the view. Since bounce rate from imported data is unavailable in this view, there's an appropriate warning, like with scroll depth. (This was actually already handled by Include bounce rate in top stats with page filter and imported data + fix 500 #6073, but listing it here for better visibility as well)time_on_pageis now shown in page-filtered Top Stats, even when imports are included in the view. There's a specific exception where the metric is dropped on the backend though -- when legacy time on page is queried in aggregate from imported data. That's because the functionality in theLegacy.TimeOnPagemodule does not exist. This was likely the reason why time on page was simply cut out of page-filtered Top Stats when imports are included.Before:
After:
Tests
Changelog
Documentation
Dark mode