CP-14167 - Add webhook health monitoring for ATS integration#76
CP-14167 - Add webhook health monitoring for ATS integration#76bernardodsanderson wants to merge 2 commits into
Conversation
What Adds periodic detection of ATS-linked accounts/partnerships missing their ATS-pointed WebhookUrl, and instruments outbound webhook delivery with New Relic metrics + Airbrake alerts on transport failures (previously swallowed silently). Why DocuSeal task completions silently stopped syncing to the ATS for ~4 months because nothing flagged the drift. These guardrails catch a missing WebhookUrl or a broken outbound delivery within hours instead of months, complementing the CP-14163 boot-time env-var guard. How to test - `CAREERPLUG_WEBHOOK_URL=... bundle exec rake careerplug:check_webhooks` — flags accounts missing the ATS webhook - `bundle exec rspec spec/lib/webhook_health_check_spec.rb spec/lib/send_webhook_request_spec.rb` - Trigger a transport error (webhook URL pointing at a dead port) and confirm Airbrake.notify fires
bernardodsanderson
requested review from
jewls618,
pollygee,
ryanarakawa and
spaulsandhu
spaulsandhu
left a comment
spaulsandhu
left a comment
There was a problem hiding this comment.
🤖 This is a semi-automated review done by Paul, with assistance from a review agent. Everything is manually signed off on, but if you feel this feedback isn't helpful, please let him know!
Clean observability slice for the DocuSeal->ATS webhook drift, and the specs are genuinely thorough. Nothing blocking from me, just a couple of questions: is the partnership scoping intentional, and what actually alerts us on a sustained non-2xx (vs a transport error)?
Nice, this is a clean little guardrail~ webhook drift going unnoticed for ~4 months is exactly the kind of thing worth instrumenting. Specs are thorough too, you covered the account/partnership/archived/blank-env branches and the 2xx / non-2xx / transport split. Also 👍🏽 on the .where.not(owner_column => nil) in the subquery (sidesteps the classic NOT IN + NULL footgun) and the defined?(::NewRelic::Agent) no-op guard.
—Paul-bot
| .where.not(external_account_id: nil) | ||
| .where.not(id: ats_webhook_owner_ids(target_sha1, :account_id)) | ||
|
|
||
| partnerships = Partnership.where.not(id: ats_webhook_owner_ids(target_sha1, :partnership_id)) |
There was a problem hiding this comment.
Just confirming my read here~ on the account side we scope to .active + external_account_id present, but Partnership gets no equivalent filter. I'm assuming that's fine because every partnership is inherently ATS-linked (external_partnership_id is not null) and there's no archived state on them, so "all partnerships missing the hook" is correct? If so, all good, just want to make sure we're not flagging partnerships that were never meant to have the ATS webhook.
There was a problem hiding this comment.
Correct read. Every Partnership is ATS-linked (external_partnership_id not null + uniqueness validation), there’s no archived/soft-delete on partnerships, and after_commit :create_careerplug_webhook always installs the ATS webhook on create. Accounts need the extra filters because they can be archived and not every account is external/ATS-linked. So “all partnerships missing the hook” is intentional.
| Rails.logger.error("Webhook transport error (#{event_type}): #{e.class} #{e.message}") | ||
| Airbrake.notify("DocuSeal outbound webhook transport error (#{event_type}): #{e.class}") | ||
| nil | ||
| else |
There was a problem hiding this comment.
[Conversational] More a question than a blocker~ a transport error pages Airbrake, but a non-2xx (say the ATS endpoint is 500ing for a sustained stretch) just records a failure metric and leans on the retry logic. If the ATS is down for a while and retries exhaust, does anything actually alert us, or is the NR dashboard the intended surface for that? Totally fine if the answer is "dashboard + retries is enough for now", I just want to make sure a prolonged outage isn't only visible if someone happens to be looking at the dashboard. 🤔
There was a problem hiding this comment.
Right — intentional split for this ticket:
• Transport errors were previously swallowed to nil with no signal → now log + Airbrake + NR failure metric.
• Non-2xx already participated in WebhookRetryLogic (retry on status >= 400 or nil, up to 10 attempts with exponential backoff). We added NR success/failure metrics so sustained non-2xx shows on the dashboard without page-spamming Airbrake on every 5xx.
After retries exhaust there is no dedicated “final failure” Airbrake today — NR is the surface for that path. A prolonged ATS 500 storm would also eventually show up on the ATS side via stuck TaskAssignment monitoring (CP-14167 ATS PR). Happy to follow up with an alert on final exhausted attempt if we want page-level signal for that path without waiting on the stuck-assignment threshold.
| exit 1 | ||
| fi | ||
|
|
||
| if ! command -v aws &> /dev/null; then |
There was a problem hiding this comment.
nitpick, not a blocker, and I'm no shell wizard: this is #!/bin/sh but command -v aws &> /dev/null uses &>, which is a bash-ism, under dash/busybox sh that parses as backgrounding the command plus a redirect, so the if ! guard wouldn't check what it looks like it checks. Same idea with the if [ $? -ne 0 ] blocks after the aws ... assignments, set -e has already bailed by then so they're effectively dead. Neither's dangerous since it fails closed either way, just flagging in case /bin/sh isn't bash in the container image.
There was a problem hiding this comment.
Good nits — image is Alpine so /bin/sh isn’t bash. Switched &> to >/dev/null 2>&1 and replaced the dead
|
Thanks Paul! Appreciate the thorough pass. |
What Hardens the one-shot rake task runner so it works under Alpine’s /bin/sh (busybox ash), not just bash. AWS CLI detection and Secrets Manager fetches now fail cleanly with portable redirects and explicit error handling. Why The webhook health check runs as a scheduled ECS task via this script. Bash-isms (&>) and dead $? checks under set -e could mis-handle errors on Alpine, so the periodic integrity check needs a portable, fail-closed startup path. How to test • sh -n bin/run_rake_task (syntax check) • Review that command -v aws uses >/dev/null 2>&1 and aws fetches use if ! VAR=$(aws ...); then • On staging after deploy: EventBridge/ECS one-shot still loads secrets and runs careerplug:check_webhooks
2 participants
CP-14167
What
Adds periodic detection of ATS-linked accounts/partnerships missing their ATS-pointed WebhookUrl, and instruments outbound webhook delivery with New Relic metrics + Airbrake alerts on transport failures (previously swallowed silently).
Why
DocuSeal task completions silently stopped syncing to the ATS for ~4 months because nothing flagged the drift. These guardrails catch a missing WebhookUrl or a broken outbound delivery within hours instead of months, complementing the CP-14163 boot-time env-var guard.
How to test
CAREERPLUG_WEBHOOK_URL=... bundle exec rake careerplug:check_webhooks— flags accounts missing the ATS webhookbundle exec rspec spec/lib/webhook_health_check_spec.rb spec/lib/send_webhook_request_spec.rb