Skip to content
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

fix: fix dropReason metrics labels #53

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

spikecurtis
Copy link

the counter_packets_dropped_reason metric is using incorrect labels, probably because a reason was dropped and a new one added to the end, causing the reasons between to be shifted out of alignment with their labels in the metrics.

The actual reasons are

const (
	dropReasonUnknownDest      dropReason = iota // unknown destination pubkey
	dropReasonUnknownDestOnFwd                   // unknown destination pubkey on a derp-forwarded packet
	dropReasonGoneDisconnected                   // destination tailscaled disconnected before we could send
	dropReasonQueueHead                          // destination queue is full, dropped packet at queue head
	dropReasonQueueTail                          // destination queue is full, dropped packet at queue tail
	dropReasonWriteError                         // OS write() failed
	dropReasonDupClient                          // the public key is connected 2+ times (active/active, fighting)
)

I noticed this while looking into DERP performance, since I was seeing a bunch of drops for gone_not_here which is not a valid reason in present version.

@spikecurtis spikecurtis merged commit 0fa29af into main May 29, 2024
6 of 35 checks passed
@andrew-d
Copy link

@spikecurtis hi there, Andrew from Tailscale here 👋 Any chance I could ask you to submit this PR upstream? Or give me permission to pick this commit into a PR, maintaining authorship information? Good find 😄

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.

3 participants