metrics: Add remaining server RPC rate metrics #15901
Conversation
tgross
changed the title
|
Looks like I've got a failing set of tests from clients getting their tokens. Moving to draft till that's fixed. |
|
Tests fixed! There's a fussy little bit around whether errors get returned wrapped with |
| - "*_endpoint.go" | ||
| - "nomad/*_endpoint.go" |
There was a problem hiding this comment.
This will exclude client endpoints, do they follow a different pattern for authorization now?
There was a problem hiding this comment.
There's two kinds of "client endpoints": the server-side client RPCs are in files like nomad/client_*_endpoint.go and get picked up with this glob. The client-side client RPCs in files like client/*_endpoint.go have always been excluded, because they don't match the first pattern with the RPC forwarding:
patterns:
- pattern: |
if done, err := $A.$B.forward($METHOD, ...); done {
return err
}
This change just makes sure we don't bother checking the files that will never have this pattern.
In #15901 we introduced pre-forwarding authentication for RPCs so that we can grab the identity for rate metrics. The `ACL.Bootstrap` RPC is an unauthenticated endpoint, so any error message from authentication is not particularly useful. This would be harmless, but if you try to bootstrap with your `NOMAD_TOKEN` already set (perhaps because you were talking to another cluster previously from the same shell session), you'll get an authentication error instead of just having the token be ignored. This is a regression from the existing behavior, so have this endpoint ignore auth errors the same way we do for every other unauthenticated endpoint (ex `Status.Peers`)
In #15901 we introduced pre-forwarding authentication for RPCs so that we can grab the identity for rate metrics. The `ACL.Bootstrap` RPC is an unauthenticated endpoint, so any error message from authentication is not particularly useful. This would be harmless, but if you try to bootstrap with your `NOMAD_TOKEN` already set (perhaps because you were talking to another cluster previously from the same shell session), you'll get an authentication error instead of just having the token be ignored. This is a regression from the existing behavior, so have this endpoint ignore auth errors the same way we do for every other unauthenticated endpoint (ex `Status.Peers`)
Continues the work done in #15876 by extending rate metrics measurement over a few stragglers that got missed in the original PR (probably because of concurrent PRs landing).
This changeset includes an update to the semgrep rule to add a rule for rate metrics, and another so that we're matching on only the currently used patterns. I've also left a some TODOs in the auth rule pointing to #15875, which isn't likely to ship in 1.5.0