Skip to content

fix: drain statsd on monitor shutdown signal#88

Closed
leno23 wants to merge 1 commit into
dolph:mainfrom
leno23:fix/monitor-shutdown-drain-statsd-issue-17
Closed

fix: drain statsd on monitor shutdown signal#88
leno23 wants to merge 1 commit into
dolph:mainfrom
leno23:fix/monitor-shutdown-drain-statsd-issue-17

Conversation

@leno23
Copy link
Copy Markdown

@leno23 leno23 commented May 17, 2026

Summary

Part of #17. monitor now handles SIGINT/SIGTERM, logs shutdown, and drains the statsd queue for up to 5s via DrainStatsd instead of exiting immediately with buffered metrics dropped.

Test plan

  • go build ./...
  • go test -short ./... -run Statsd

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70224bc113

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread connectivity.go
Comment on lines +179 to +180
log.Print("Received shutdown signal, draining statsd...")
DrainStatsd(config, 5*time.Second)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Stop metric producers before draining queue

MonitorLoop starts a drain after receiving SIGINT/SIGTERM, but it never stops the dest.Monitor() goroutines that continue calling dest.Check() and enqueueing statsd payloads. Because DrainStatsd exits as soon as the queue is momentarily empty, shutdown can return while producers are still active, so metrics generated during shutdown are still dropped despite the new drain path.

Useful? React with 👍 / 👎.

Comment thread statsd.go
Comment on lines +63 to +64
case s := <-queue:
emitStatsd(config, s)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Bound each drain send by the shutdown timeout

DrainStatsd is documented to flush for up to timeout, but inside the loop it performs a blocking send via emitStatsd; that path uses net.Dial without any deadline. If the statsd endpoint is slow or unreachable, a single drain iteration can block well past 5 seconds, preventing timely process termination after the shutdown signal.

Useful? React with 👍 / 👎.

Part of dolph#17. Monitor waits for SIGINT/SIGTERM, then flushes queued statsd
metrics for up to five seconds before exiting instead of dropping them.
@leno23 leno23 force-pushed the fix/monitor-shutdown-drain-statsd-issue-17 branch from 70224bc to 0365d01 Compare May 17, 2026 14:27
@dolph dolph closed this 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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants