add more labels to RequestRecorder #12727

acpana · 2022-04-08T00:08:40Z

overview

This PR adds more labels to the RequestRecorder. The following are mandatory labels:

leader = {true | false | unreported}

The optional labels, given duck typing, are:

blocking = {true | false}
target_datacenter = {string}
locality = {forwarded | local}

These labels allow further observability for the new rpc metric(s).

todos left:

docs update (separate PR) --> add docs for new labels #12757
changelog
ent shadow PR/ make sure it merges

Co-authored-by: Daniel Nephin <[email protected]> Signed-off-by: FFMMM <[email protected]>

Signed-off-by: FFMMM <[email protected]>

agent/consul/server.go

acpana · 2022-04-08T00:10:29Z

agent/rpc/middleware/interceptors.go

+	labelsArr := flattenLabels(labels)
+	r.Logger.Trace(requestLogName, labelsArr...)


i did my best to not add another helper func here -- let me know if there's a easier method to enumerate keys, values as []interface{} in go 🙈

eculver

LGTM, just a few non-blocking suggestions.

agent/rpc/middleware/interceptors_test.go

agent/consul/server.go

eculver · 2022-04-08T02:45:49Z

agent/consul/server_test.go

@@ -1344,11 +1347,13 @@ func TestServer_RPC_MetricsIntercept(t *testing.T) {
 			{Name: "errored", Value: "false"},
 			{Name: "request_type", Value: "read"},
 			{Name: "rpc_type", Value: "test"},
+			{Name: "server_role", Value: "unreported"},


Shouldn't this get wired up with the test's (*Server).IsLeader func and thus not be "unreported"?

hm, that's a fair point; the way i was thinking is that this kind of test needs to happen at the middleware layer 🤔

also agent/metrics_test.go should take care of the server_role/ e2e.

dhiaayachi

Thank you for working on this @FFMMM. I have few comments related to the use of IsLeader mostly.

dhiaayachi · 2022-04-11T16:38:28Z

agent/consul/server.go

@@ -425,6 +404,29 @@ func NewServer(config *Config, flat Deps, publicGRPCServer *grpc.Server) (*Serve
 		fsm:                     newFSMFromConfig(flat.Logger, gc, config),
 	}

+	var recorder *middleware.RequestRecorder
+	if flat.NewRequestRecorderFunc != nil {
+		recorder = flat.NewRequestRecorderFunc(serverLogger, s.IsLeader, s.config.Datacenter)


I'm not sure what are the implication of passing IsLeader to the request recorder in here. IsLeader go as deep as atomically reading the raft state to be able to tell and that could have sides effects that are hard to reason about (specially performance effects)
I would recommend to have a separate state for the recorder that we can update when we acquire or loose leadership.

I would recommend to have a separate state for the recorder that we can update when we acquire or loose leadership.

Would you mind chatting thru this some more with me? When I thought about this, I started to think that it can get too complex keeping tabs on the leadership state for a server but I may be wrong here 🤔

On another note, I see this "pattern" (of passing the func/ a smaller interface) being loosely used in gateway code here:

consul/agent/consul/gateway_locator.go

Lines 272 to 277 in 09d61e6

type serverDelegate interface {

blockingQuery(queryOpts blockingQueryOptions, queryMeta blockingQueryResponseMeta, fn queryFn) error

IsLeader() bool

LeaderLastContact() time.Time

setDatacenterSupportsFederationStates()

}

We were chatting offline and believe that the underlying raft.getState() per RPC call would not adversely impact performance.

We are still interested in coming up with a different way of the server telling the request recorder about its leadership state. I will backlog an issue for this.

agent/consul/server.go

agent/rpc/middleware/interceptors.go

dhiaayachi · 2022-04-11T16:50:20Z

agent/consul/server_test.go

 		}

 		key := keyMakingFunc(middleware.OneTwelveRPCSummary[0].Name, expectedLabels)

 		if _, ok := storage[key]; !ok {
+			// the compound key will look like: "rpc+server+call+Status.Ping+false+read+test+unreported"


maybe we can simplify the content of the key and use assertion to verify the values in here. I think in the test you can most of the time infer the right entry without having all of this in the key.

I think we need the name of the rpc call (otherwise the key gets overwritten in the map).

The labels help distinguish the happy path for us so we don't "fail silently" on the calls and mark the test as passed.

mkeeler · 2022-04-11T18:56:29Z

agent/rpc/middleware/interceptors.go


 	labels := []metrics.Label{
 		{Name: "method", Value: requestName},
 		{Name: "errored", Value: strconv.FormatBool(respErrored)},
 		{Name: "request_type", Value: reqType},
 		{Name: "rpc_type", Value: rpcType},
+		{Name: "server_role", Value: serverRole},


nit/bikeshed:

Suggested change

{Name: "server_role", Value: serverRole},

{Name: "leader", Value: strconv.FormatBool(isLeader)},

RPC metrics will only be emitted by servers so there is no need to prefix the metric name with server. Secondly since there are only two possible values of leader and follower I think the metric might be more intuitive for someone exploring their grafana or prometheus to have the name be leader and the value be a boolean.

this makes a lot of sense! and that's what I am changing for a new commit ⌨️

I think it would be nice to keep unreported, and subsequently keep this as a tri state

leader: {`true`, `false`, `unreported`}

for a defense in depth kind of approach. If the function is not passed to the RequestRecorder, I wouldn't think it useful or accurate to report leader: false

Signed-off-by: FFMMM <[email protected]>

…re-labels-4

hc-github-team-consul-core · 2022-04-12T17:51:03Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/635231.

hc-github-team-consul-core · 2022-04-12T17:51:05Z

🍒✅ Cherry pick of commit a46bbe8 onto release/1.12.x succeeded!

Co-authored-by: Daniel Nephin <[email protected]> Signed-off-by: FFMMM <[email protected]>

FFMMM added 3 commits April 7, 2022 15:41

add more labels: dc, leader, read attr

cfe9458

Co-authored-by: Daniel Nephin <[email protected]> Signed-off-by: FFMMM <[email protected]>

plumb to server, add unit tests, server tests

f8cd22d

Signed-off-by: FFMMM <[email protected]>

enumertate labels in logger

e87c458

Signed-off-by: FFMMM <[email protected]>

acpana commented Apr 8, 2022

View reviewed changes

agent/consul/server.go Outdated Show resolved Hide resolved

acpana commented Apr 8, 2022

View reviewed changes

acpana requested a review from a team April 8, 2022 00:11

eculver approved these changes Apr 8, 2022

View reviewed changes

agent/rpc/middleware/interceptors_test.go Show resolved Hide resolved

agent/consul/server.go Outdated Show resolved Hide resolved

agent/consul/server.go Outdated Show resolved Hide resolved

eculver reviewed Apr 8, 2022

View reviewed changes

dhiaayachi reviewed Apr 11, 2022

View reviewed changes

mkeeler reviewed Apr 11, 2022

View reviewed changes

acpana added the backport/1.12 label Apr 11, 2022

review: add errored test, rename label to leader, changelog

90b5513

Signed-off-by: FFMMM <[email protected]>

vercel bot temporarily deployed to Preview – consul April 11, 2022 20:18 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging April 11, 2022 20:18 Inactive

acpana requested a review from dhiaayachi April 11, 2022 20:23

acpana mentioned this pull request Apr 11, 2022

add docs for new labels #12757

Merged

Merge remote-tracking branch 'upstream/main' into ffmmm/rpc-improv-mo…

4650b87

…re-labels-4

vercel bot deployed to Preview – consul April 12, 2022 16:43 View deployment

vercel bot deployed to Preview – consul-ui-staging April 12, 2022 16:43 View deployment

acpana merged commit a46bbe8 into main Apr 12, 2022

acpana deleted the ffmmm/rpc-improv-more-labels-4 branch April 12, 2022 17:50

hc-github-team-consul-core pushed a commit that referenced this pull request Apr 12, 2022

add more labels to RequestRecorder (#12727)

9eb0137

Co-authored-by: Daniel Nephin <[email protected]> Signed-off-by: FFMMM <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more labels to RequestRecorder #12727

add more labels to RequestRecorder #12727

acpana commented Apr 8, 2022 •

edited

Loading

acpana Apr 8, 2022

eculver left a comment

eculver Apr 8, 2022

acpana Apr 11, 2022

dhiaayachi left a comment

dhiaayachi Apr 11, 2022

acpana Apr 11, 2022

acpana Apr 11, 2022

dhiaayachi Apr 11, 2022

acpana Apr 11, 2022 •

edited

Loading

mkeeler Apr 11, 2022

acpana Apr 11, 2022

hc-github-team-consul-core commented Apr 12, 2022

hc-github-team-consul-core commented Apr 12, 2022

		labelsArr := flattenLabels(labels)
		r.Logger.Trace(requestLogName, labelsArr...)

	type serverDelegate interface {
	blockingQuery(queryOpts blockingQueryOptions, queryMeta blockingQueryResponseMeta, fn queryFn) error
	IsLeader() bool
	LeaderLastContact() time.Time
	setDatacenterSupportsFederationStates()
	}

	{Name: "server_role", Value: serverRole},
	{Name: "leader", Value: strconv.FormatBool(isLeader)},

add more labels to RequestRecorder #12727

add more labels to RequestRecorder #12727

Conversation

acpana commented Apr 8, 2022 • edited Loading

overview

todos left:

Choose a reason for hiding this comment

eculver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhiaayachi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acpana Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hc-github-team-consul-core commented Apr 12, 2022

hc-github-team-consul-core commented Apr 12, 2022

acpana commented Apr 8, 2022 •

edited

Loading

acpana Apr 11, 2022 •

edited

Loading