Quiet the "No more splits" log noise for draining-style stages - #124
Open
nusense wants to merge 2 commits into
Open
Quiet the "No more splits" log noise for draining-style stages#124nusense wants to merge 2 commits into
nusense wants to merge 2 commits into
Conversation
get_dataset_for() re-raised StopIteration (from a split type run out of data, e.g. nfiles.peek()) as a bare AssertionError. For stages configured with default_clear_cronjob=False (expected to keep polling for new files to show up later), this fires on every unsuccessful cron poll, and poms_method's error_rewrite has no specific handling for it, so it falls through to the generic catch-all and logs a full ERROR-level traceback via logging.exception on each occurrence -- for what is a routine, expected "nothing new yet" condition, not a bug. Introduce StagesPOMS.NoMoreSplits (a plain Exception subclass) and raise that instead of AssertionError, then give it its own except clause in poms_method.error_rewrite that logs a short line via logit.log instead of a full traceback, while still returning the same HTTPError(400, ...) to the client as before. Deliberately scoped to this one condition rather than a blanket `except AssertionError`, since AssertionError is used throughout the codebase for genuine bug/misconfiguration checks that should keep their full traceback logging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The quiet NoMoreSplits log line previously just repeated the generic "No more splits in this campaign." text, giving no way to tell which campaign/stage it was about. Build the message from camp (the CampaignStage passed into get_dataset_for) instead: campaign name, stage name, and campaign_stage_id. Same message now shows up both in the quiet log line and in the HTTPError response to the caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
It would be hard to test this on the integration instance because it requires running campaigns with splits that are then exhausted. The code itself seems straightforward though. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_dataset_for()re-raises a split type'sStopIteration(e.g.nfiles.peek()when a dataset has been fully sliced up) as a bareAssertionError("No more splits in this campaign."). For stages configured withdefault_clear_cronjob=False(expected to keep polling for new files to show up later, e.g. draining-style datasets), this fires on every unsuccessful cron poll, forever, until new data arrives or someone intervenes.poms_method'serror_rewritehas no specific handling forAssertionError, so it falls through to the generic catch-all, which logs a full ERROR-level traceback vialogging.exceptionon every occurrence — noisy, repeated log spam for what is a routine, expected "nothing new yet" condition, not a bug:Fix
StagesPOMS.NoMoreSplits(a plainExceptionsubclass) and raise that instead ofAssertionError. Deliberately scoped to this one condition rather than a blanketexcept AssertionError, sinceAssertionErroris used throughout the codebase for genuine bug/misconfiguration checks (e.g.assert self.init_shrek_if_needed(ctx), "...") that should keep their full traceback logging.NoMoreSplitsits ownexceptclause inpoms_method.error_rewrite, positioned before the generic catch-all: logs a short line vialogit.loginstead of a full traceback, while still returning the sameHTTPError(400, ...)to the client as before."No more splits in campaign '<name>' stage '<name>' (campaign_stage_id=<id>)."— instead of the old generic "No more splits in this campaign." with no way to tell which one. Same message shows up both in the quiet log line and in the HTTP response.Test plan
default_clear_cronjob=False, and confirm the log shows a single line naming the campaign/stage instead of a full traceback.default_clear_cronjob=Truestill has its cron schedule cleared as before (unaffected by this change).AssertionErrors elsewhere in the app still get full traceback logging as before.🤖 Generated with Claude Code