Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom event timeseries in stats API #3505

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

salomvary
Copy link
Contributor

@salomvary salomvary commented Nov 8, 2023

This is a followup of the question I asked in #3495.

Meanwhile, we also contacted support via email, to which the answer was:

It's not possible to query for the "events" metric in the timeseries endpoint, because you cannot get total pageviews + total custom events for a specific timeframe broken down by days. This is not possible in the dashboard either and the API follows the dashboard.

Which, well, confirms that it is not possible to get a timeseries of custom events, but does not explain the reason.

I poked around in the codebase and it seems to be possible to get timeseries of custom events with minimal changes. Note that these are my first two lines in Elixir and I have no idea what I am doing :)

I did some manual testing with a locally running Plausible, and this had the expected outcome. If these changes are deemed acceptable, I'm happy to figure out how to add tests, documentation & co.

I've tested this locally withe the following:

mix send_pageview --event 'click' --props '{"test-property": "42"}'
mix send_pageview --event 'click' --props '{"test-property": "42"}'

curl -X GET --location "http://localhost:8000/api/v1/stats/timeseries?site_id=dummy.site&period=7d&metrics=pageviews%2Cevents&interval=date&filters=event%3Aprops%3Atest-property%3D%3D42" \
    -H "Authorization: Bearer ZURiZABY2RXdYuD39TSKPCFeP8HjXcPvaFl9bJ3bqXgtTn6-87HuAsM4JHoa7gG2" | jq . 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   342  100   342    0     0   4890      0 --:--:-- --:--:-- --:--:--  5428
{
  "results": [
    {
      "date": "2023-11-02",
      "events": 0,
      "pageviews": 0
    },
    {
      "date": "2023-11-03",
      "events": 0,
      "pageviews": 0
    },
    {
      "date": "2023-11-04",
      "events": 0,
      "pageviews": 0
    },
    {
      "date": "2023-11-05",
      "events": 0,
      "pageviews": 0
    },
    {
      "date": "2023-11-06",
      "events": 0,
      "pageviews": 0
    },
    {
      "date": "2023-11-07",
      "events": 0,
      "pageviews": 0
    },
    {
      "date": "2023-11-08",
      "events": 2,
      "pageviews": 0
    }
  ]
}

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@salomvary salomvary marked this pull request as draft November 8, 2023 18:43
@salomvary
Copy link
Contributor Author

@ukutaht @vinibrsl I guess you are all busy shipping Business subscription features, but if someone has a few minutes to give feedback on whether it's worth continuing here, that would be highly appreciated. 🙏

@ukutaht
Copy link
Contributor

ukutaht commented Nov 10, 2023

Yes I think the limitation at the moment if fairly artificial. The main downside is that we cannot include imported data in the events metric but I think that's OK. I'm curious about your use-case for this feature?

We can integrate this PR if:

  1. The metric is available on aggregate and breakdown endpoints in addition to timeseries
  2. It's tested and documented properly

@salomvary
Copy link
Contributor Author

Thanks for feedback!

Yes I think the limitation at the moment if fairly artificial. The main downside is that we cannot include imported data in the events metric but I think that's OK. I'm curious about your use-case for this feature?

We are building a rudimentary ad service for a website where we already use Plausible for traffic analytics. The clicks on the ads are tracked as custom events, and the (numeric) identifier of the ad is added as a custom property to the event.

This way we are hoping to be able to answer questions using Plausible's stats API like "how many times was a given ad clicked in total", or "what was the distribution of clicks for each day since the ad is live". The latter is currently not possible, hence this change proposal.

We can integrate this PR if:

  1. The metric is available on aggregate and breakdown endpoints in addition to timeseries

According to the docs, aggregate and breakdown already support events metric. (For breakdown I even have first-hand experience.)

  1. It's tested and documented properly

Yep, happy to do that, but wanted to confirm the change is sensible beforehand.

Have a nice weekend all:)

salomvary added a commit to salomvary/plausible-docs that referenced this pull request Nov 13, 2023
@salomvary salomvary force-pushed the timeseries-for-events branch 2 times, most recently from 0c2d7aa to 8199c4a Compare November 13, 2023 07:02
@salomvary salomvary marked this pull request as ready for review November 13, 2023 07:10
@salomvary
Copy link
Contributor Author

@ukutaht I've added test & documentation and believe that this is ready for review.

@ukutaht
Copy link
Contributor

ukutaht commented Nov 14, 2023

@salomvary build failed with a linting error. The check can be disabled for the function like so:

# credo:disable-for-next-line Credo.Check.Refactor.CyclomaticComplexity

@salomvary
Copy link
Contributor Author

@ukutaht Pushed the fix to the linting error.

@lakrisgubben
Copy link

Just wanted to chime in here that this functionality would be immensely helpful for us as well. We've got a similar use case as @salomvary, i.e. a rudimentary banner system using Plausible events to track impressions/clicks. We'd use this API functionality to create graphs to show impressions/clicks for each day.

Thank you @salomvary for working on this! :)

@vinibrsl vinibrsl merged commit 555eb25 into plausible:master Nov 17, 2023
5 checks passed
@vinibrsl
Copy link
Contributor

Thank you!

@salomvary
Copy link
Contributor Author

Seems like this is live already \o/

Thanks for open-sourcing Plausible, it's been a pleasure contributing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants