Skip to content

Prevent WikiMetrics from collecting metrics if db record already exists - #1268

Open
dati18 wants to merge 6 commits into
mainfrom
fix-pdo-error
Open

dati18 wants to merge 6 commits into
mainfrom
fix-pdo-error

Conversation

@dati18

@dati18 dati18 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Prevent WikiMetrics from collecting metrics if a record for today already exists in the database, as that would result in a primary key integrity constraint violation.

This should help reduce logs like this:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '321_2026-01-01' for key 'PRIMARY'

Bug: T423554

Comment thread app/Metrics/App/WikiMetrics.php Outdated
Comment thread app/Console/Kernel.php Outdated
Comment thread app/Console/Kernel.php Outdated
@outdooracorn

outdooracorn commented Sep 29, 2026 •

Copy link
Copy Markdown
Member
  • Add a test that runs the UpdateWikiDailyMetricJob() twice to reproduc the test
  • Add withoutOverlapping() to the job to prevent 2 runs at the same job from overlapping
  • Add a check for existing records for this wiki/date to WikiMetrics.php

Bug: T423554

The current description gives an overview of what this PR does, which is useful but ultimately can likely be figured out by looking at the diff. What would be really useful to add to the description is why we are we making this change. What was the original context/issue? Why does this change achieve its goal? Etc.

P.S. there are also some typos in the description ;)

Comment thread tests/Jobs/UpdateWikiDailyMetricJobTest.php
@dati18
dati18 force-pushed the fix-pdo-error branch 2 times, most recently from 74bebe7 to 3ac45dc Compare September 29, 2026 15:35

@outdooracorn outdooracorn left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks correct. However, I have some suggested improvements.

Also, the PR description still talks about the "schedule without overlapping" change that was removed from this PR.

I see you're on annual leave now, so I'll take over this PR.


I think that another possible reason we are getting this "Integrity constraint violation" log spam is because the 'redis' queue has a retry_after of only 100 seconds but UpdateWikiDaiyMetricJob::$timeout is set to 1hr - meaning that the job can be retried by another queue worker before the job is expected to have finished or has been terminated. The API chart sets Horizon tries to 3 by default so jobs are retied by default. I think the ShouldBeUnique interface is only evaluated at dispatch time, not when a job is retried. I think Job Middleware is run so we could use the WithoutOverlapping middleware (not to be confused with Schedule::withoutOverlapping() which is named similarly but functions completely differently).

Comment thread tests/Jobs/UpdateWikiDailyMetricJobTest.php Outdated
Comment thread app/Metrics/App/WikiMetrics.php
Comment thread app/Metrics/App/WikiMetrics.php
Comment thread tests/Jobs/UpdateWikiDailyMetricJobTest.php Outdated
Comment thread tests/Jobs/UpdateWikiDailyMetricJobTest.php Outdated
Comment thread tests/Jobs/UpdateWikiDailyMetricJobTest.php Outdated
@outdooracorn outdooracorn changed the title Fix duplicate daily wiki metrics by making the write idempotent Prevent WikiMetrics from saving metrics if db record already exists Oct 6, 2026
@outdooracorn outdooracorn changed the title Prevent WikiMetrics from saving metrics if db record already exists Prevent WikiMetrics from collecting metrics if db record already exists Oct 6, 2026

@outdooracorn outdooracorn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good to go from my side but I touched it last so will let someone else also approve.

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.

3 participants