Bishnu/ep level retry timeout http clients #823

bishnuag · 2022-08-04T12:13:52Z

ERD:
https://docs.google.com/document/d/1e6Nmhoa5ygsh2lXSzUyJdHqz2pXF0lc5ACr8DzwKBio/edit#

Summary:

Let the user pass TimeoutAndRetry parameters per request to the client (HTTP and channel)
If TimeoutAndRetry parameters per request are not passed, client-level TimeoutAndRetry parameters is taken into consideration
Runtime considers the least timeout value among clients and request level

fyi: The branch reads it as the changes are only for HTTP clients but its for both HTTP and TChannel

CLAassistant · 2022-08-04T12:13:57Z

All committers have signed the CLA.

coveralls · 2022-08-10T08:58:02Z

Coverage decreased (-0.7%) to 69.288% when pulling 5dd6816 on bishnu/ep-level-retry-timeout-http-clients into d9a8b55 on master.

runtime/client_http_request.go

isopropylcyanide · 2022-10-26T18:15:44Z

codegen/templates/http_client.tmpl

 	qpsLevels := map[string]string{
 				{{range $methodName, $qpsLevel := $QPSLevels -}}
 				"{{$methodName}}": "{{$qpsLevel}}",
 				{{end}}
 	}
 	if !circuitBreakerDisabled {
-		for methodName := range methodNames {
+		for methodName, methodTimeoutVal := range clientMethodTimeoutMapping{


Nit: Prefer methodTimeout for brevity.

Suggested change

for methodName, methodTimeoutVal := range clientMethodTimeoutMapping{

for methodName, methodTimeout := range clientMethodTimeoutMapping{

make sense, addressed

codegen/templates/tchannel_endpoint.tmpl

isopropylcyanide · 2022-10-26T18:18:31Z

codegen/templates/workflow.tmpl

@@ -16,6 +16,9 @@ package workflow
 {{- $serviceMethod := printf "%s%s" (title .Method.ThriftService) (title .Method.Name) }}
 {{- $workflowInterface := printf "%sWorkflow" $serviceMethod }}
 {{- $workflowStruct := camel $workflowInterface }}
+{{- $endpointId := .Spec.EndpointID }}
+{{- $handleId := .Spec.HandleID }}
+{{- $handleIdDotEndpointId := printf "%s.%s" ($endpointId) ($handleId) }}


Suggested change

{{- $handleIdDotEndpointId := printf "%s.%s" ($endpointId) ($handleId) }}

{{- $handleIdDotEndpointIdFmt := printf "%s.%s" ($endpointId) ($handleId) }}

isopropylcyanide · 2022-10-26T18:19:25Z

codegen/templates/workflow.tmpl

+	var backOffTimeAcrossRetries time.Duration
+	var overAllTimeout time.Duration
+
+	//when endpoint level timeout information provided, override it with client level config


Suggested change

//when endpoint level timeout information provided, override it with client level config

//when endpoint level timeout information is available, override it with client level config

isopropylcyanide · 2022-10-26T18:20:08Z

codegen/templates/workflow.tmpl

+		scaleFactor := w.defaultDeps.Config.MustGetFloat("endpoints.{{$handleIdDotEndpointId}}.scaleFactor")
+		maxRetry = int(w.defaultDeps.Config.MustGetInt("endpoints.{{$handleIdDotEndpointId}}.retryCount"))
+
+		backOffTimeAcrossRetriesConf := w.defaultDeps.Config.MustGetInt("endpoints.{{$handleIdDotEndpointId}}.backOffTimeAcrossRetries")


Suggested change

backOffTimeAcrossRetriesConf := w.defaultDeps.Config.MustGetInt("endpoints.{{$handleIdDotEndpointId}}.backOffTimeAcrossRetries")

backOffTimeAcrossRetriesCfg := w.defaultDeps.Config.MustGetInt("endpoints.{{$handleIdDotEndpointId}}.backOffTimeAcrossRetries")

isopropylcyanide · 2022-10-26T18:20:35Z

codegen/templates/workflow.tmpl

+		//add a scale factor to timeoutPerAttempt
+		timeoutPerAttemptConf = int64(math.Ceil(float64(timeoutPerAttemptConf) * scaleFactor))
+
+		//convert timeoutPerAttemptConf & backOffTimeAcrossRetries to Duration


Nit: Redundant comment

Moved it to utils , redundant code for all workflow

isopropylcyanide · 2022-10-26T18:21:30Z

codegen/templates/workflow.tmpl

+		timeoutPerAttempt = time.Duration(timeoutPerAttemptConf) * time.Millisecond
+		backOffTimeAcrossRetries = time.Duration(backOffTimeAcrossRetriesConf) * time.Millisecond
+
+		overAllTimeout = time.Duration((timeoutPerAttemptConf + backOffTimeAcrossRetriesConf) * (int64(maxRetry-1)) + timeoutPerAttemptConf) * time.Millisecond


We don't need to have this logic in tmpl This should be sourced from the runtime. tmpl should only be the invoker or contain branching at max. This makes our template bloated and increases code gen size.

Moved it to utils , redundant code for all workflow

isopropylcyanide · 2022-10-26T18:22:17Z

examples/example-gateway/clients/multi/client-config.yaml

-    HelloA: ServiceABack::hello
-    HelloB: ServiceBBack::hello
+    HelloA: ServiceABack::helloA
+    HelloB: ServiceBBack::helloB


Doesn't it change the test? Two clients map to the same procedure vs two clients mapping to different procedure

Yes, it does a bit :). Have updated it to not alter the test case. Details -
In the new code I have updated the QPS level for both helloA and helloB as QPS level is mapped as "clientID-clientMehtod" but in this case we have multiple services for client multi, so the mapping breaks fundamentally.

To fix this completely, we have to name the circuit breaker name as "clientId-serviceName-serviceMethod" - Will have a backlog ticket for now.

Got it. Can you create a Github issue for this and reference this PR there? Otherwise we'll lose context.

isopropylcyanide · 2022-10-26T18:26:04Z

runtime/options.go

+
+import "time"
+
+// TimeoutAndRetryOptions specifies timeout and retries params for outbound calls


Nice practice of adding an options here. However, we could explore a option/decorator pattern similar to what is being done in fx and glue:

https://github.com/uber-go/fx/blob/dd3b312175bccf69a9eca00d856cc1d129f69525/docs/ex/annotate/cast.go#L59

isopropylcyanide

Initial review

isopropylcyanide

LGTM assuming that the retry functionality has been extensively tested across sample/example gateways across both (ep level / client level) timeouts.

bennytran-uber · 2022-10-31T02:42:52Z

codegen/templates/tchannel_client.tmpl

@@ -143,6 +143,17 @@ func {{$exportName}}(deps *module.Dependencies) Client {
 		{{ end -}}
 	}

+	//get mapping of client method and it's timeout
+	//if mapping is not provided, use client's timeout for all the methods
+	clientMethodTimeoutMapping := make(map[string]int64)


why did we decide to make the values int64 here? If we used an int instead, we could get rid of the type conversion inside configureCircuitBreaker

int doesn't for deps.Default.Config.MustGetStruct("clients.{{$clientID}}.methodTimeoutMapping", &clientMethodTimeoutMapping) statement, hence had to update it int64

bennytran-uber · 2022-10-31T02:48:33Z

runtime/client_http_request.go

 	}
+	req.ContextLogger.Debug(req.ctx, "ClientHTTPRequest definition",
+		zap.String("clientId", req.ClientID), zap.String("methodName", req.MethodName),
+		zap.String("req.RequestTimeoutPerAttemptInMs", string(rune(req.timeoutAndRetryOptions.RequestTimeoutPerAttemptInMs))),


why not use zap.Int instead to avoid the type conversion?

Make sense, Addressed

bennytran-uber · 2022-10-31T02:49:58Z

runtime/client_http_request.go

 	if err != nil {
-		req.ContextLogger.ErrorZ(req.ctx, "Could not make outbound request", zap.Error(err))
-		return nil, err
+		req.ContextLogger.ErrorZ(req.ctx, fmt.Sprintf("Could not make http outbound %s.%s request",


is the formatting in the error log message necessary? Shouldn't we get this information through some of the logger tags from the context logger?

Ideally, it should have come through the context logger but it wasn't the case. As a quick fix I added it here instead debugging it

bennytran-uber · 2022-10-31T02:51:22Z

runtime/client_http_request.go

+		req.ContextLogger.GetLogger().Info("errors while making http outbound request",
+			zap.Error(err),
+			zap.String("clientId", req.ClientID), zap.String("methodName", req.MethodName),
+			zap.String("attempt", string(rune(retryCount))),


same question here, we can use zap.Int types?

bennytran-uber · 2022-10-31T02:52:06Z

runtime/client_http_request.go

+			shouldRetry = req.client.CheckRetry(ctx, req.timeoutAndRetryOptions, res, err)
+		}
+
+		req.ContextLogger.GetLogger().Info("errors while making http outbound request",


should be an error log?

also why did we expose the underlying logger instead of using the context logger directly?

Have updated it as Warn as retries could be pending which says it's not the final error.
Why not contextLogger? Each attempt may fail with diff reason and may override the message in contextLogger, what we want is individual logs for each retry. Other approach is to fix contextLogger to have same field with multiple values, like msg1, msg2..

bennytran-uber · 2022-10-31T03:01:11Z

runtime/utils.go

+)
+
+//GetTimeoutAndRetryConfig encapsulates timeout and retry configs in TimeoutAndRetryOptions struct
+func GetTimeoutAndRetryConfig(timeoutPerAttemptConf int, backOffTimeAcrossRetriesConf int,


nit: BuildTimeoutAndRetryConfig might be a better name?

:), addressed

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch 4 times, most recently from 6ad31ac to bc66b21 Compare August 10, 2022 10:44

bishnuag commented Aug 17, 2022

View reviewed changes

runtime/client_http_request.go Outdated Show resolved Hide resolved

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch 9 times, most recently from 90fd389 to 0601f99 Compare September 12, 2022 06:56

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch from 0601f99 to 910cafa Compare September 13, 2022 05:42

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch from 910cafa to 86a6b90 Compare September 21, 2022 10:46

bishnuag requested review from zeeshan-mohd and anirudhraja September 21, 2022 10:51

zeeshan-mohd mentioned this pull request Oct 2, 2022

New release failing the build on zanzibar uber/tchannel-go#889

Closed

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch 9 times, most recently from 386c480 to 04df52c Compare October 10, 2022 20:12

isopropylcyanide reviewed Oct 26, 2022

View reviewed changes

codegen/templates/tchannel_endpoint.tmpl Outdated Show resolved Hide resolved

isopropylcyanide reviewed Oct 26, 2022

View reviewed changes

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch 6 times, most recently from 6a0b9fa to ea4ffc5 Compare October 27, 2022 18:01

anirudhraja approved these changes Oct 28, 2022

View reviewed changes

isopropylcyanide approved these changes Oct 28, 2022

View reviewed changes

bennytran-uber reviewed Oct 31, 2022

View reviewed changes

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch 2 times, most recently from 270e8c6 to 0a47bbd Compare November 2, 2022 09:39

ep level timeout and retry support

5dd6816

bishnuag force-pushed the bishnu/ep-level-retry-timeout-http-clients branch from 0a47bbd to 5dd6816 Compare November 2, 2022 10:14

bennytran-uber approved these changes Nov 3, 2022

View reviewed changes

bishnuag merged commit 3984088 into master Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bishnu/ep level retry timeout http clients #823

Bishnu/ep level retry timeout http clients #823

bishnuag commented Aug 4, 2022 •

edited

Loading

CLAassistant commented Aug 4, 2022 •

edited

Loading

coveralls commented Aug 10, 2022 •

edited

Loading

isopropylcyanide Oct 26, 2022

bishnuag Oct 27, 2022

isopropylcyanide Oct 26, 2022

bishnuag Oct 27, 2022

isopropylcyanide Oct 26, 2022 •

edited

Loading

bishnuag Oct 27, 2022

isopropylcyanide Oct 26, 2022

bishnuag Oct 27, 2022

isopropylcyanide Oct 26, 2022

bishnuag Oct 27, 2022

isopropylcyanide Oct 26, 2022

bishnuag Oct 27, 2022

isopropylcyanide Oct 26, 2022

bishnuag Oct 27, 2022

isopropylcyanide Oct 28, 2022

bishnuag Oct 28, 2022

isopropylcyanide Oct 26, 2022 •

edited

Loading

isopropylcyanide left a comment

isopropylcyanide left a comment

bennytran-uber Oct 31, 2022

bishnuag Oct 31, 2022

bennytran-uber Oct 31, 2022

bishnuag Oct 31, 2022

bennytran-uber Oct 31, 2022

bishnuag Oct 31, 2022

bennytran-uber Oct 31, 2022

bishnuag Oct 31, 2022

bennytran-uber Oct 31, 2022

bennytran-uber Oct 31, 2022

bishnuag Oct 31, 2022

bennytran-uber Oct 31, 2022

bishnuag Oct 31, 2022

	for methodName, methodTimeoutVal := range clientMethodTimeoutMapping{
	for methodName, methodTimeout := range clientMethodTimeoutMapping{

	{{- $handleIdDotEndpointId := printf "%s.%s" ($endpointId) ($handleId) }}
	{{- $handleIdDotEndpointIdFmt := printf "%s.%s" ($endpointId) ($handleId) }}

	//when endpoint level timeout information provided, override it with client level config
	//when endpoint level timeout information is available, override it with client level config

	backOffTimeAcrossRetriesConf := w.defaultDeps.Config.MustGetInt("endpoints.{{$handleIdDotEndpointId}}.backOffTimeAcrossRetries")
	backOffTimeAcrossRetriesCfg := w.defaultDeps.Config.MustGetInt("endpoints.{{$handleIdDotEndpointId}}.backOffTimeAcrossRetries")


		import "time"

		// TimeoutAndRetryOptions specifies timeout and retries params for outbound calls

Bishnu/ep level retry timeout http clients #823

Bishnu/ep level retry timeout http clients #823

Conversation

bishnuag commented Aug 4, 2022 • edited Loading

CLAassistant commented Aug 4, 2022 • edited Loading

coveralls commented Aug 10, 2022 • 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

isopropylcyanide Oct 26, 2022 • 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

isopropylcyanide Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

isopropylcyanide left a comment

Choose a reason for hiding this comment

isopropylcyanide 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

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

bishnuag commented Aug 4, 2022 •

edited

Loading

CLAassistant commented Aug 4, 2022 •

edited

Loading

coveralls commented Aug 10, 2022 •

edited

Loading

isopropylcyanide Oct 26, 2022 •

edited

Loading

isopropylcyanide Oct 26, 2022 •

edited

Loading