Skip to content

Move Top Stats to new internal stats endpoint#6035

Open
RobertJoonas wants to merge 12 commits intomasterfrom
internal-api-refactor
Open

Move Top Stats to new internal stats endpoint#6035
RobertJoonas wants to merge 12 commits intomasterfrom
internal-api-refactor

Conversation

@RobertJoonas
Copy link
Contributor

@RobertJoonas RobertJoonas commented Jan 28, 2026

Changes

This PR introduces a new POST /api/stats/:domain/query endpoint and makes the Top Stats use it. The endpoint will eventually replace all the Api.StatsController actions such as main_graph, sources, pages, etc.

  • To call the new endpoint, there's a new stats function in api.ts. It takes a StatsQuery object which aims to mimic the query params required to construct %ParsedQueryParams{} on the backend.
  • In order to build the StatsQuery object, there's also a new function called createStatsQuery which takes DashboardState and ReportParams objects 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:

  1. bounce_rate is 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)
  2. time_on_page is 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 the Legacy.TimeOnPage module 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.
  3. Top Stats time labels (the ones rendered only in comparison mode) do not span into the future anymore. E.g. if today is Feb 2nd, then:

Before:

image

After:

image

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@github-actions
Copy link

Preview environment👷🏼‍♀️🏗️
PR-6035

@RobertJoonas RobertJoonas force-pushed the internal-api-refactor branch 2 times, most recently from 329200e to 1fcedf4 Compare February 2, 2026 15:45
@RobertJoonas RobertJoonas marked this pull request as draft February 3, 2026 10:01
@RobertJoonas RobertJoonas marked this pull request as ready for review February 19, 2026 13:26
@RobertJoonas RobertJoonas changed the title [WIP] New internal stats endpoint Top Stats from /api/stats/:domain/query Feb 19, 2026
@RobertJoonas RobertJoonas changed the title Top Stats from /api/stats/:domain/query Move Top Stats to new internal stats endpoint Feb 19, 2026
@RobertJoonas RobertJoonas requested review from apata and zoldar February 19, 2026 13:53
with {:ok, compare} <- parse_include_compare(params["include"]) do
{:ok,
%QueryInclude{
imports: params["include"]["imports"] !== false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@RobertJoonas RobertJoonas Feb 19, 2026

Choose a reason for hiding this comment

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

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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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! 😄

Copy link
Contributor

@apata apata left a comment

Choose a reason for hiding this comment

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

Works well!

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.

4 participants