feat(xds): add xdsNACKTotal metric - #9205
Conversation
Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…d-xdsnacktotal-metric
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f538a4671c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9205 +/- ##
==========================================
+ Coverage 74.90% 75.05% +0.14%
==========================================
Files 252 252
Lines 40815 40819 +4
==========================================
+ Hits 30574 30638 +64
+ Misses 8158 8091 -67
- Partials 2083 2090 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
| @@ -258,6 +258,7 @@ func (s *snapshotCache) OnStreamRequest(streamID int64, req *discoveryv3.Discove | |||
| // TODO(youngnick): Handle NACK properly | |||
There was a problem hiding this comment.
As you prefer, I let it there as I didn't consider that handling NACKs only through exposing those as a metric was enough to consider that they're now handled properly. Especially since I'm planning on opening follow-up PRs to actually do that.
But if you think that it's ok to remove it, I can just do that
There was a problem hiding this comment.
Ok nvm, after reading arkdog's comment, I just updated the comment so that it's the same as with OnStreamDeltaRequest
| if status := req.ErrorDetail; status != nil { | ||
| // if Envoy rejected the last update log the details here. | ||
| // TODO(youngnick): Handle NACK properly | ||
| // Envoy rejected (NACKed) the last update. It keeps serving its last known |
There was a problem hiding this comment.
can we also add this to OnStreamRequest (in case we ever flip to SoTW)
There was a problem hiding this comment.
I already have the metric added to OnStreamRequest but I just added the same explanatory comment there. Is that what you were suggesting ?
Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
…d-xdsnacktotal-metric
* feat(xds): add xdsNACKTotal metric Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io> * add release-note Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
* feat(xds): add xdsNACKTotal metric Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io> * add release-note Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
* feat(xds): add xdsNACKTotal metric Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io> * add release-note Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
* feat(xds): add xdsNACKTotal metric Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io> * add release-note Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io> Signed-off-by: Andrea Abellonio <andrea.abellonio@servescale.ai>
3 participants
What type of PR is this?
feat(xds): this PR adds the
xdsNACKTotalmetric which was written as a TODO in a comment along the relevant test.What this PR does / why we need it:
As described in the issue below, currently NACK isn't propagated to the EnvoyProxy or Gateway statuses which make the pods silently fail. This PR is a first step towards the proper handling of NACK by exposing the total number of xds NACKs as a counter type metric. This will at least allow users to create alerts based on it to know when a config is broken.
Which issue(s) this PR fixes:
Fixes (partly): #9198
I'll open (a) follow-up PR(s) to further address the above issue.