GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat(celery): allow custom monitor config for beat tasks by alexon1234 · Pull Request #7839 · getsentry/sentry-python ConversationDescriptionCelery Beat auto-instrumentation ( Consequence: a task that can legitimately run longer than Sentry's default 30-minute This adds a import sentry_sdk from sentry_sdk.integrations.celery import CeleryIntegration sentry_sdk.init( integrations=[ CeleryIntegration( monitor_beat_tasks=True, beat_task_monitor_config={ "nightly-cleanup": {"max_runtime": 120, "checkin_margin": 10}, }, ), ], ) The SDK still derives Changes:
IssuesReminders
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsCelery Beat auto-instrumentation derives the cron monitor config from the schedule only, so max_runtime, checkin_margin, failure_issue_threshold, recovery_threshold and owner cannot be set. Tasks whose runtime can exceed Sentry's default 30-minute max_runtime are then reported as timed-out cron failures even when they complete successfully. Add a `beat_task_monitor_config` option to CeleryIntegration that maps beat task names to partial monitor configs, merged over the derived config. Refs getsentry#7838
ericapisani
requested changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your contribution! This is a great start, I've left some notes that we'll need to tackle before this gets merged in. Let me know if you have any questions about what I've left here.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we need a
Suggested change
This would allow us to update the integration.beat_task_monitor_config.get(monitor_name) or getattr(integration.beat_task_monitor_config, monitor_name, None) depending on your preference.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove this test
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Comment on lines
+478
to
+479
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Within the SDK, we're trying to move towards testing functionality via the public APIs rather than the private methods and use mocks as sparingly as we can. Instead of this approach, can we look to instead do something similar to the
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't support overriding the schedule (or its type) via this new property as there's validation that we do within the SDK to ensure the provided values are valid. Instead, let's do the following: 1️⃣ Introduce a new type in 2️⃣ Validate the overrides in the In _ALLOWED_MONITOR_CONFIG_OVERRIDE_KEYS = frozenset( { "checkin_margin", "max_runtime", "failure_issue_threshold", "recovery_threshold", "owner", } ) def _validate_beat_task_monitor_config( beat_task_monitor_config: "Dict[str, MonitorConfigOverrides]", ) -> None: for task_name, overrides in beat_task_monitor_config.items(): invalid = set(overrides) - _ALLOWED_MONITOR_CONFIG_OVERRIDE_KEYS if invalid: raise ValueError( f"Unsupported keys in beat_task_monitor_config for '{task_name}': " f"{sorted(invalid)}. Allowed keys: " f"{sorted(_ALLOWED_MONITOR_CONFIG_OVERRIDE_KEYS)}" ) and then invoke
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsThis branch has not been deployedNo deployments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
ReviewersRequested changes must be addressed to merge this pull request. Uh oh!
There was an error while loading. Please reload this page. AssigneesNo one assignedLabels
None yet
ProjectsMilestoneNo milestoneDevelopmentSuccessfully merging this pull request may close these issues. Celery Beat auto-instrumentation: allow configuring monitor limits (max_runtime, checkin_margin, ...)Uh oh!
There was an error while loading. Please reload this page. 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. | ||||||||||||||||||||||||||||||||||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. |