Skip to content

Move concurrent update error logging to debug level.#477

Merged
antoine-le-calloch merged 2 commits into
mainfrom
move_ConcurrentAuxUpdate_error_to_debug
May 18, 2026
Merged

Move concurrent update error logging to debug level.#477
antoine-le-calloch merged 2 commits into
mainfrom
move_ConcurrentAuxUpdate_error_to_debug

Conversation

@antoine-le-calloch

Copy link
Copy Markdown
Contributor

No description provided.

@antoine-le-calloch antoine-le-calloch self-assigned this May 15, 2026
Copilot AI review requested due to automatic review settings May 15, 2026 17:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Concurrent aux-update errors were being logged as errors by the #[instrument(..., err)] attribute, creating noise during normal operation since these are expected and gracefully handled via fallback. This PR downgrades logging for ConcurrentAuxUpdate to debug level while preserving error-level logging for other failures.

Changes:

  • Remove err from #[instrument] on update_aux_inner across ZTF, LSST, and DECAM workers.
  • Explicitly match on the error and log ConcurrentAuxUpdate at debug level, other errors at error level.
  • Remove the redundant warn! in base.rs when concurrent modification is detected.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/alert/ztf.rs Drop err from instrument; log concurrent update at debug, others at error.
src/alert/lsst.rs Same change applied to LSST worker.
src/alert/decam.rs Same change applied to DECAM worker.
src/alert/base.rs Remove redundant warn log at the source of ConcurrentAuxUpdate.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

Copy link
Copy Markdown

Throughput results (39374164bda0cfea3cf27ab66e0674a4d9dc334a):

New wall time Baseline wall time Difference
244.0 245.1 0%

@Theodlz

Theodlz commented May 18, 2026

Copy link
Copy Markdown
Collaborator

@antoine-le-calloch I kind of feel like this warning/error should not show up if all was working properly, meaning instead of just silencing a genuine warning maybe we should:

  • run a script to fix all the existing data in the DB, so this doesn't happen anymore
  • if it still does after the DB-wide patch, investigate what's happening

@antoine-le-calloch

Copy link
Copy Markdown
Contributor Author

@antoine-le-calloch I kind of feel like this warning/error should not show up if all was working properly, meaning instead of just silencing a genuine warning maybe we should:

* run a script to fix all the existing data in the DB, so this doesn't happen anymore

* if it still does after the DB-wide patch, investigate what's happening

@Theodlz I think you're confusing it with the other is not strictly increasing error. I have also a PR open for this one but I'm still working on it.

@Theodlz

Theodlz commented May 18, 2026

Copy link
Copy Markdown
Collaborator

@antoine-le-calloch I kind of feel like this warning/error should not show up if all was working properly, meaning instead of just silencing a genuine warning maybe we should:

* run a script to fix all the existing data in the DB, so this doesn't happen anymore

* if it still does after the DB-wide patch, investigate what's happening

@Theodlz I think you're confusing it with the other is not strictly increasing error. I have also a PR open for this one but I'm still working on it.

ohhh my bad, you are right. I did mix up these 2 errors. Let me re-read this PR.

@Theodlz Theodlz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. That approach where we don't always log errors in the function and instead do it manually isn't my favorite, but it's fine here given the length of the function! Feel free to merge (once green)

@antoine-le-calloch

Copy link
Copy Markdown
Contributor Author

LGTM. That approach where we don't always log errors in the function and instead do it manually isn't my favorite, but it's fine here given the length of the function! Feel free to merge (once green)

Yeah I agree but how can I move it to debug without doing this?

@github-actions

Copy link
Copy Markdown

Throughput results (bcd73245d81af9b45c066076e6bc2e6d4f6edf59):

Storage New wall time Baseline wall time Difference
mongo 247.6 245.8 0%
s3 286.4 271.6 5.00%

@Theodlz

Theodlz commented May 18, 2026

Copy link
Copy Markdown
Collaborator

LGTM. That approach where we don't always log errors in the function and instead do it manually isn't my favorite, but it's fine here given the length of the function! Feel free to merge (once green)

Yeah I agree but how can I move it to debug without doing this?

we could imagine an approach where the function that's currently raising the error (that really is just a warning) returns some Status enum instead of just Ok() (a little bit like we handle the "alert already exist" stuff, if you ever looked at that). But honestly, I don't think it's worth the extra complexity here. We can stick with what you added!

@antoine-le-calloch antoine-le-calloch merged commit dacff21 into main May 18, 2026
6 checks passed
@antoine-le-calloch antoine-le-calloch deleted the move_ConcurrentAuxUpdate_error_to_debug branch May 18, 2026 14:45
@github-project-automation github-project-automation Bot moved this to Done in BOOM Dev May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants