Conversation
2f968b8 to
9cbb218
Compare
|
I would be a bit cautious with using context in our 'hot path' as there is quite a performance impact related to it as it uses runtime reflection to work. Is it possible to have the information about the connection string being the 'local' propagated to gocbcorex such that we can directly (internally to gocbcorex) discover whether a node we are routing to is system-local or server-group-local? |
|
@brett19 Have changed it to make the address of the local node be a part of the crud component and propagated at startup, so should address the expensive context concerns. |
aa6183a to
2dac69b
Compare
brett19
left a comment
There was a problem hiding this comment.
Looks good, although I think it might be good to include both 'local' as well as 'server group' local, since in most deployments the latter will be most common.
07729c7 to
8a5c4dd
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for routing metrics that track whether KV requests are serviced locally, within the same server group, or remotely. The implementation propagates node locality information through the system and increments appropriate metrics based on endpoint selection.
Changes:
- Added
NodesWatcherHttpcomponent to monitor cluster nodes and their server groups - Extended configuration structs to include local node address and server group information
- Updated routing orchestration to track and categorize memd requests using OpenTelemetry metrics
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| agent_options.go | Added GetNodes function, LocalNodeAddr, and ServerGroup fields to support routing metrics |
| agent.go | Initialized local KV endpoint and configured CRUD component with routing information |
| bucketstracking_agentmanager.go | Integrated NodesWatcherHttp to track node topology and server groups |
| nodeswatcher_http.go | New component that watches cluster nodes and maintains hostname-to-server-group mappings |
| streamwatcher_http.go | Added streamWatcherHttp_streamNodes function to stream node configuration updates |
| crud.go | Added GetServerGroupEndpoints method and propagated routing parameters through CRUD operations |
| vbucketrouter.go | Updated routing orchestration to increment metrics based on endpoint locality |
| metrics.go | Defined three new metrics counters for local, server group, and remote requests |
| contrib/cbconfig/cbconfig.go | Added ServerGroup field to node configuration JSON |
| crud_test.go | Updated test to pass empty routing parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8a5c4dd to
ca1704d
Compare
705abab to
38ff298
Compare
chvck
left a comment
There was a problem hiding this comment.
Looks good to me, will leave +2 to Brett.
38ff298 to
780293f
Compare
| return emptyResp, err | ||
| } | ||
|
|
||
| if localKvEp == endpoint { |
There was a problem hiding this comment.
Rather than having the metrics incremented through the vbucketrouter which is meant to be a simple reusable component, I think this should happen in one of the orchestration levels of gocbcorex proper.
There was a problem hiding this comment.
It's worth considering the 'hot path' here as well. If the user does not have metrics enabled, is this somewhere we have context to be able to disable it?
7b147ba to
2ccafad
Compare
557d26e to
bc7e18b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bc7e18b to
5da0011
Compare
5da0011 to
1d6b716
Compare
30f1aa5 to
a9ae6ef
Compare
brett19
left a comment
There was a problem hiding this comment.
Approved. It would be good to merge your microbenchmarking code first to validate that this does not have a significant negative effect on performance though.
a9ae6ef to
348974b
Compare
This PR adds support for metrics that track the performance of optimal routing. This is done by:
In order to have a minimal impact on the performance of crud ops the actual metric emission is done off the hot path. The endpoint resolved is sent down a channel to a metric worker which reads the contents of this channel and emits the appropriate metric.