-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(cleanup): Minor changes #101558
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
base: master
Are you sure you want to change the base?
ref(cleanup): Minor changes #101558
Conversation
Changes included: * Start transaction for each process * Add metric to track process errors
|
||
return | ||
|
||
model_name, chunk = j |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is easier to review by hiding the white spaces:
https://github.com/getsentry/sentry/pull/101558/files?diff=split&w=1
logger.exception("Error in multiprocess_worker.") | ||
finally: | ||
task_queue.task_done() | ||
with sentry_sdk.start_transaction(op="cleanup", name="multiprocess_worker") as transaction: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finally: | ||
task_queue.task_done() | ||
with sentry_sdk.start_transaction(op="cleanup", name="multiprocess_worker") as transaction: | ||
transaction.set_tag("model", model_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will help me distinguish the impact per model.
if not task.chunk(apply_filter=True): | ||
break | ||
except Exception: | ||
metrics.incr("cleanup.error", instance=model_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we can create monitors for this. We should not let things get as bad as they currently are.
is_flag=True, | ||
help="Send the duration of this command to internal metrics.", | ||
hidden=True, | ||
help="(deprecated) Send the duration of this command to internal metrics.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All scripts are passing the --timed
parameter, thus, we don't need this.
# main process tracks the overall cleanup operation performance. | ||
with sentry_sdk.start_transaction(op="cleanup", name="cleanup") as transaction: | ||
transaction.set_tag("router", router) | ||
transaction.set_tag("model", model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All errors are happening within the processes, thus, these taggings are useless.
I will remove the other one on a different commit/PR to save triggering CI.
Changes included: