Metrics namespace change #499

herainman · 2018-10-23T21:27:02Z

This commits:

Use RootScope instead of PerHostScope
Rename counter and timer
Add protocol tag for HTTP and TChannel

coveralls · 2018-10-23T21:50:54Z

Coverage increased (+0.02%) to 61.772% when pulling 040487c on alignnamespace into c662e77 on master.

coveralls · 2018-10-23T21:50:54Z

Coverage increased (+4.6%) to 66.308% when pulling 41f5c5a on alignnamespace into 2f9c950 on master.

ChuntaoLu · 2018-10-23T23:42:14Z

runtime/call_metrics.go

+	endpointScope = "endpoint"
+
+	inboundCallsRecvd        = "request"
+	inboundCallsLatency      = "totalhandlertime"


I think "latency" is probably better.

Agreed. Totalhandlertime is to align with RTAPI

We don't have to align just for the sake of alignment. The name totalhandlertime is historical and shouldn't be inherited moving forward.

We don't have to align just for the sake of alignment. The name totalhandlertime is historical and shouldn't be inherited moving forward.

fixed

Did you forget to push to remote? Still showing totalhandlertime to me.

ChuntaoLu · 2018-10-23T23:43:15Z

runtime/call_metrics.go

-	outboundCallsErrors       = "outbound.calls.errors"
-	outboundCallsStatus       = "outbound.calls.status"
+	outboundCallsSent         = "request"
+	outboundCallsLatency      = "routerhandlertime"


This should also just be "latency".

Agreed. routerhandlertime is to align with RTAPI

ChuntaoLu · 2018-10-23T23:46:56Z

runtime/call_metrics.go

+	outboundCallsAppErrors    = "app-errors"
+	outboundCallsSystemErrors = "system-errors"
+	outboundCallsErrors       = "errors"
+	outboundCallsStatus       = "status"


Now that the metrics are sub scoped with client or endpoint, the values of these metrics become call direction agnostic, we don't need to have two set of different variables to represent the same metric names, e.g. - inboundCallsSuccess and outboundCallsSuccess should just be consolidated to success.

ChuntaoLu · 2018-10-24T17:49:42Z

runtime/gateway.go

@@ -476,11 +473,11 @@ func (gateway *Gateway) setupMetrics(config *StaticConfig) (err error) {
 	gateway.AllHostScope = gateway.RootScope.SubScope(


Is AllHostScope needed anymore?

AllHostScope is needed since it is related to zap namespace and tchannel metrics namespace, however, metrics would only have one scope which is RootScope

AllHostScope is needed since it is related to zap namespace and tchannel metrics namespace

Related in what way? Can you elaborate?

AllHostScope is needed since it is related to zap namespace and tchannel metrics namespace

Related in what way? Can you elaborate?

https://github.com/uber/zanzibar/blob/master/runtime/gateway.go#L556-L560
https://github.com/uber/zanzibar/blob/master/runtime/gateway.go#L619-L622
https://github.com/uber/zanzibar/blob/master/runtime/gateway.go#L580-L596
https://github.com/uber/zanzibar/blob/master/runtime/gateway.go#L682-L684

All these references can be replaced by the RootScope, the AllHostScope metric name service + "." + env + ".all-workers" is redundant and verbose, it is already tagged with env and service. I believe the original purpose for its existence is to reduce the cardinality introduced by tags host and dc, which I don't know if it is still a valid concern. If we want to have the same effect, we can replace it the RootScope before it is tagged with perHostTags, not the one that is overwritten here https://github.com/uber/zanzibar/pull/499/files#diff-ee4b13d64c39fd8e3d0483c1076bb31cR480.

ChuntaoLu · 2018-10-24T17:55:12Z

This is a breaking change in a sense that dashboards built on previous metrics will no longer work. We need to put this in the changelog and communicate.

herainman · 2018-10-26T21:54:56Z

Merged master, please review, thank you

ChuntaoLu · 2018-10-26T22:04:13Z

runtime/metrics_namespace.go

+	clientSuccess   = "client.success"
+	clientStatus    = "client.status"
+	clientErrors    = "client.errors"
+	clientAppErrors = "client.app-errors"


Shouldn't there be a clientSystemErrors = "client.system-errors"?

ChuntaoLu · 2018-10-26T22:08:13Z

runtime/tchannel_client.go

@@ -271,13 +272,13 @@ func (c *tchannelOutboundCall) finish(ctx context.Context, err error) {
 		errCause := tchannel.GetSystemErrorCode(errors.Cause(err))
 		scopeTags := map[string]string{scopeTagError: errCause.MetricsKey()}
 		ctx = WithScopeTags(ctx, scopeTags)
-		c.metrics.IncCounter(ctx, outboundCallsSystemErrors, 1)
+		c.metrics.IncCounter(ctx, clientErrors, 1)


This should be clientSystemErrors

This reverts commit 19aa43d.

herainman force-pushed the alignnamespace branch from e26940c to 040487c Compare October 23, 2018 21:42

herainman requested review from ChuntaoLu and jacobgreenleaf October 23, 2018 21:46

ChuntaoLu reviewed Oct 24, 2018

View reviewed changes

ChuntaoLu mentioned this pull request Oct 24, 2018

ContextMetrics #488

Merged

herainman force-pushed the alignnamespace branch 3 times, most recently from bfbfc13 to a89637d Compare October 26, 2018 21:54

ChuntaoLu reviewed Oct 26, 2018

View reviewed changes

Metrics namespace change

41f5c5a

herainman force-pushed the alignnamespace branch from a89637d to 41f5c5a Compare October 26, 2018 22:56

ChuntaoLu approved these changes Oct 26, 2018

View reviewed changes

herainman merged commit 19aa43d into master Oct 26, 2018

herainman added a commit that referenced this pull request Oct 27, 2018

Revert "Metrics namespace change (#499)"

f643381

This reverts commit 19aa43d.

herainman added a commit that referenced this pull request Oct 27, 2018

Revert "Metrics namespace change (#499)"

af33c2e

This reverts commit 19aa43d.

herainman deleted the alignnamespace branch October 27, 2018 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics namespace change #499

Metrics namespace change #499

herainman commented Oct 23, 2018 •

edited

Loading

coveralls commented Oct 23, 2018

coveralls commented Oct 23, 2018 •

edited

Loading

ChuntaoLu Oct 23, 2018

herainman Oct 24, 2018

ChuntaoLu Oct 24, 2018

herainman Oct 25, 2018

ChuntaoLu Oct 25, 2018

ChuntaoLu Oct 23, 2018

herainman Oct 24, 2018

herainman Oct 25, 2018

ChuntaoLu Oct 25, 2018

ChuntaoLu Oct 23, 2018

herainman Oct 25, 2018

ChuntaoLu Oct 24, 2018

herainman Oct 24, 2018 •

edited

Loading

ChuntaoLu Oct 24, 2018

herainman Oct 25, 2018 •

edited

Loading

ChuntaoLu Oct 25, 2018

herainman Oct 25, 2018

ChuntaoLu commented Oct 24, 2018

herainman commented Oct 26, 2018

ChuntaoLu Oct 26, 2018

herainman Oct 26, 2018

ChuntaoLu Oct 26, 2018

herainman Oct 26, 2018

		@@ -476,11 +473,11 @@ func (gateway Gateway) setupMetrics(config StaticConfig) (err error) {
		gateway.AllHostScope = gateway.RootScope.SubScope(

Metrics namespace change #499

Metrics namespace change #499

Conversation

herainman commented Oct 23, 2018 • edited Loading

coveralls commented Oct 23, 2018

coveralls commented Oct 23, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herainman Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herainman Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChuntaoLu commented Oct 24, 2018

herainman commented Oct 26, 2018

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

herainman commented Oct 23, 2018 •

edited

Loading

coveralls commented Oct 23, 2018 •

edited

Loading

herainman Oct 24, 2018 •

edited

Loading

herainman Oct 25, 2018 •

edited

Loading