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

Feature/slotted counts #2074

Merged
merged 19 commits into from
Mar 28, 2024
Merged

Conversation

acelaya
Copy link
Member

@acelaya acelaya commented Mar 26, 2024

Closes #2036

TODO

  • Visits count is tracked via a preFlush event listener. This is usually fine as long as the short URL existed before the visit, but in certain cases this is not true, like in tests, where we have to remember flushing short URLs before visits, or when importing short URLs.
    This can be solved by using a combination of onFlush to track the entities to be created + postFlush, where those will have IDs. For some reason, when I first checked this I oversaw the existence of these events.
  • Test performance impact of tracking counts when a visit occurs, for every database engine, and also with a lot of concurrent traffic.
  • Try to find a native way to do upserts in MS SQL and SQLite which does not require a select followed by an insert or update.
    I tried using a MERGE statement, but it ended up running un race conditions with duplicate keys due to not properly locking rows. See https://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/?utm_source=AaronBertrand
    Existing SELECT then INSERT or UPDATE approach works better.
  • Sometimes API test covering short URLs lists fail when ordering by title.
    This is already happening with non-postgres engines in develop branch. It was just found at this moment because I tried to run API tests with the other engines.
  • Test again both new migrations when coming from a big dataset
    • MySQL/Maria
    • Postgres
    • MS SQL Server
    • SQLite
  • Test importing short URLs and visits from an instance with a lot of both.
    • MySQL/Maria
    • Postgres
    • MS SQL Server
    • SQLite
  • Visits tracking should now be wrapped in a transaction, so that visits and visits_count creation are all or nothing.
  • Consider adding a database test for the new ShortUrlVisitsCountTracker

Next steps

  • Try to take advantage of this feature for tags with stats. See Improve performance when sorting tags with stats by visits or short URLs count #1346
  • If the above shows a clear improvement, consider adding an endpoint for domains with stats.
  • Consider tracking orphan visits counts in the same way.
  • Count visits using the new approach for visits overviews.
  • Fix the API test around sorting short URLs, which fails only with non-postgress databases.
  • Generate code coverage for all database tests and merge them, not just SQLite, as otherwise there's no full coverage for ShortUrlVisitsCountTracker

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 95.93496% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 97.30%. Comparing base (b4c46ce) to head (8cb5d44).
Report is 3 commits behind head on develop.

Files Patch % Lines
.../src/Visit/Listener/ShortUrlVisitsCountTracker.php 94.11% 4 Missing ⚠️
...dule/Core/src/Visit/Entity/ShortUrlVisitsCount.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2074      +/-   ##
=============================================
- Coverage      97.35%   97.30%   -0.06%     
- Complexity      1343     1357      +14     
=============================================
  Files            250      252       +2     
  Lines           4772     4852      +80     
=============================================
+ Hits            4646     4721      +75     
- Misses           126      131       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acelaya acelaya marked this pull request as ready for review March 28, 2024 15:49
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.

Track amount of visits per short URL so that we don't have to do COUNT aggregates at runtime
1 participant