Handle multiple services in generated http and tchannel clients #594

mattgode · 2019-05-15T02:22:05Z

(Changed from previous version to reflect changes)

In this diff, we add a new parameter which is the client's target endpoint. For a thrift-based http client, this would be in the form of ServiceName::methodName. This is to provide support in metrics and logging for thrift files that define multiple services. Clients for such thrift files have cases where the client method name cannot match the thrift method name. Consider the following service definitions in one thrift file for a single client:

service Foo {
    string fn1()
}

service Bar {
    string fn1()
}

Client method names cannot simply be the method name or we would have conflicting method names for the generated client struct. Thus, we must concatenate the service and method names to make this work. For example. FooFn1 and BarFn1.

…vices per client

coveralls · 2019-05-15T17:18:10Z

Coverage remained the same at 66.79% when pulling b5d451e on mattgode:mgode/handle-multiple-services into 8a984b0 on uber:master.

ChuntaoLu · 2019-05-15T16:40:29Z

runtime/client_http_request.go

+	scopeTags := map[string]string{
+		scopeTagClientMethod:  methodName,
+		scopeTagClient:        clientID,
+		scopeTagClientService: serviceName,


The scopeTagClientService tag, which is "clientservice", is a bit ambiguous as it doesn't differentiate between service name (as in service discovery) and Thrift service name. A better tag would be scopeTagsTargetEndpoint, whose value should be thrift service::method, e.g. Bar::helloWorld. It would also align with TChannel client metrics tag.

To achieve that, we will need to pass in a thriftServiceMethod parameter to the NewClientHTTPRequest and adding the field to ClientHTTPRequest. The downside of such change is slightly leaky abstraction as the client http request is now aware of the outer layer thrift service and method. A better alternative is that we move tag creation into generated code since all of the tags we add here are static info regarding the client and are available in the template.

But I am okay with either way because changing the signature isn't a really big deal since it is only used in the generated code.

ChuntaoLu · 2019-05-15T16:46:45Z

runtime/client_http_request.go

@@ -53,17 +54,23 @@ type ClientHTTPRequest struct {
 // NewClientHTTPRequest allocates a ClientHTTPRequest. The ctx parameter is the context associated with the outbound requests.
 func NewClientHTTPRequest(
 	ctx context.Context,
-	clientID, methodName string,


The methodName here is the Go method name of the corresponding client. We can keep it and introduce a new param thrfitServiceMethod.

I was hoping to get away with not making a breaking change and rely on backwards compatibility. I just went ahead and made a breaking change as you suggested and passed the (thrift) service method names as another argument.

ChuntaoLu · 2019-05-15T16:54:45Z

runtime/http_client.go

@@ -54,7 +54,7 @@ func NewHTTPClient(
 	logger *zap.Logger,
 	scope tally.Scope,
 	clientID string,
-	methodNames []string,
+	serviceMethodNames []string,


We can change the param type form []string to map[string]string to capture both the Go method name of the client that user provides and its corresponding thrift service and method name.

ChuntaoLu · 2019-05-15T17:21:30Z

runtime/client_http_request.go

@@ -168,7 +175,7 @@ func (req *ClientHTTPRequest) WriteJSON(

 // Do will send the request out.
 func (req *ClientHTTPRequest) Do() (*ClientHTTPResponse, error) {
-	opName := fmt.Sprintf("%s.%s", req.ClientID, req.MethodName)
+	opName := fmt.Sprintf("%s.%s", req.ServiceName, req.MethodName)


If we do change the client request struct, then ("%s.%s(%s)", req.ClientID, req.MethodName, req.ThriftServiceMethod) might be a good alternative.

ChuntaoLu · 2019-05-15T21:56:39Z

runtime/http_client.go

@@ -55,6 +55,7 @@ func NewHTTPClient(
 	scope tally.Scope,
 	clientID string,
 	methodNames []string,
+	targetEndpointNames []string,


I think it makes more sense to have a map, whose key is client method name and value is corresponding thrift serviceMethod, that replace both methodNames and targetEndpointNames. That is essentially what the user-provided exposedMethods is.

With that, the template change would be changing https://github.com/uber/zanzibar/blob/master/codegen/templates/http_client.tmpl#L103-L107 to

map[string]string{ {{range $serviceMethod, $methodName := $exposedMethods -}} "{{$methodName}}": "{{$serviceMethod}}", {{end}} },

ChuntaoLu · 2019-05-15T22:10:26Z

runtime/context.go

-	scopeTagsTargetService  = "targetservice"
-	scopeTagsTargetEndpoint = "targetendpoint"
+	scopeTagClientMethod         = "clientmethod"
+	scopeTagClientTargetEndpoint = "clienttargetendpoint"


It is probably not necessary to introduce this new tag, scopeTagsTargetEndpoint ("targetendpoint") should be sufficient because of the existing contexts for the client operation. Also tchannel client and http client sharing this same tas make it easier when we query by tags.

ChuntaoLu · 2019-05-15T22:12:25Z

codegen/templates/tchannel_client.tmpl

@@ -102,7 +102,7 @@ func {{$exportName}}(deps *module.Dependencies) Client {
 		{{$serviceMethod := printf "%s::%s" $svc.Name .Name -}}
 		{{$methodName := (title (index $exposedMethods $serviceMethod)) -}}
 			{{if $methodName -}}
-			"{{$serviceMethod}}": "{{$methodName}}",
+			"{{$serviceMethod}}": "{{title .Name}}",


This should not be changed, the $methodName is supposed to be the Go method name of the client.

For the Edge Gateway usage of this template, there was always the assumption that the client and method names were a 1-1 mapping. Multiple services in one thrift disallow this. After talking offline with Chuntao offline, it sounds like we are going to just scrap the tchannel part of this diff and make consumers of metrics and logs switch to other metrics to avoid breaking changes in metrics and logging here.

cc @abhishekparwal FYI

For the Edge Gateway usage of this template, there was always the assumption that the client and method names were a 1-1 mapping. Multiple services in one thrift disallow this. After talking offline with Chuntao offline, it sounds like we are going to just scrap the tchannel part of this diff and make consumers of metrics and logs switch to other metrics to avoid breaking changes in metrics and logging here.

To clarify, consumers of the metrics/logs will still look at the same metrics/logs but different tag/log field for thrift service and method. The client method name has always meant to be the Go method name of the client.

runtime/http_client.go

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

ChuntaoLu

Sorry, forgot suggesting the test changes. I have verified tests pass with these changes. You can also batch commit the suggestions thru the UI.

runtime/client_http_request_test.go

test/endpoints/bar/bar_metrics_test.go

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

mattgode added 2 commits May 14, 2019 16:51

Change http and tchannel clients to better handle multiple thrift ser…

f2b503d

…vices per client

Fix test cases

4840882

mattgode force-pushed the mgode/handle-multiple-services branch from aeff8b7 to 4840882 Compare May 15, 2019 02:38

mattgode requested review from jacobgreenleaf and ChuntaoLu May 15, 2019 02:41

Fix branch coverage

d229116

ChuntaoLu reviewed May 15, 2019

View reviewed changes

Address code review comments

3132caa

mattgode force-pushed the mgode/handle-multiple-services branch from 311a970 to 3132caa Compare May 15, 2019 20:53

ChuntaoLu reviewed May 15, 2019

View reviewed changes

Do not change tchannel client

f9264da

ChuntaoLu reviewed May 16, 2019

View reviewed changes

runtime/http_client.go Outdated Show resolved Hide resolved

runtime/http_client.go Outdated Show resolved Hide resolved

runtime/http_client.go Outdated Show resolved Hide resolved

mattgode and others added 3 commits May 16, 2019 09:51

Update runtime/http_client.go

9990c73

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

Update runtime/http_client.go

cb4db75

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

Update runtime/http_client.go

6fa93c2

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

ChuntaoLu reviewed May 16, 2019

View reviewed changes

runtime/client_http_request_test.go Outdated Show resolved Hide resolved

test/endpoints/bar/bar_metrics_test.go Outdated Show resolved Hide resolved

mattgode and others added 2 commits May 16, 2019 11:22

Update test/endpoints/bar/bar_metrics_test.go

fd29ab0

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

Update runtime/client_http_request_test.go

b5d451e

Co-Authored-By: skiptomylu <chuntaolu13@gmail.com>

ChuntaoLu approved these changes May 16, 2019

View reviewed changes

ChuntaoLu merged commit 3d0daab into uber:master May 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle multiple services in generated http and tchannel clients #594

Handle multiple services in generated http and tchannel clients #594

mattgode commented May 15, 2019 •

edited

Loading

coveralls commented May 15, 2019 •

edited

Loading

ChuntaoLu May 15, 2019

ChuntaoLu May 15, 2019

mattgode May 15, 2019

ChuntaoLu May 15, 2019

ChuntaoLu May 15, 2019

ChuntaoLu May 15, 2019

ChuntaoLu May 15, 2019

ChuntaoLu May 15, 2019

mattgode May 15, 2019

mattgode May 15, 2019

ChuntaoLu May 16, 2019

ChuntaoLu left a comment

Handle multiple services in generated http and tchannel clients #594

Handle multiple services in generated http and tchannel clients #594

Conversation

mattgode commented May 15, 2019 • edited Loading

coveralls commented May 15, 2019 • 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

ChuntaoLu left a comment

Choose a reason for hiding this comment

mattgode commented May 15, 2019 •

edited

Loading

coveralls commented May 15, 2019 •

edited

Loading