go-resty / resty

Simple HTTP and REST client library for Go
MIT License
10.21k stars 713 forks source link

Middleware around resty.Request.Execute including retries #788

Open rob0rt opened 7 months ago

rob0rt commented 7 months ago

I'm in the process of instrumenting one of my applications with OTel, and was able to inject the OTel tracing client through resty.NewWithClient(otelhttp.DefaultClient) which gives me spans around each individual request (similar to resty's OnBeforeRequest and OnAfterRequest), but it would be nice if I could wrap all of the requests made with my resty client with a singular span that encompass all retries.

In practice, I think this would look like adding a middleware dispatch at the beginning and end of request.go:Execute (https://github.com/go-resty/resty/blob/v2/request.go#L900-L968)

I don't have any good suggestions for naming, but maybe something like OnBeforeRetryableRequest and OnAfterRetryableRequest?

This would, concretely, allow me to instrument my resty client as follows:

client := resty.NewWithClient(otelhttp.DefaultClient) // otelhttp instruments each individual request

// Create a span which will wrap all subsequent requests
client.OnBeforeRetryableRequest(func(c *resty.Client, req *resty.Request) error {
  ctx, _ := tracer.Start(req.Context(), req.Method+" "+req.URL)
  req.SetContext(ctx)
  return nil
})

// End the span which wraps all retries
client.OnAfterRetryableRequest(func(c *resty.Client, res *resty.Response) error {
  span := trace.SpanFromContext(res.Request.Context())
  span.End()
  return nil
})

which might product a trace like the following:

[ GET https://example.com ..................................................... ]
 [ HTTP GET ............. ]                [ HTTP GET (retry) ....... ]

Currently this is possible, but requires instrumenting all individual usages of the resty client as follows:

func GetExampleCom(ctx context.Context, c *resty.Client) {
  ctx, span := tracer.Start(req.Context(), "GET https://example.com")
  defer span.End()

  c.R().Get("https://example.com")
}

Having to do this at all callsites isn't impossible, but it would be nice to make it implicit behavior for my resty client instance.

jeevatkm commented 6 months ago

@rob0rt Thanks for reaching out and proposing suggestions. It does make sense. I'm putting my effort into v3. Can you try these two functions? Let me know if they work for the use case. Based on your feedback, I will plan the design for v3 accordingly. Thanks again.

jeevatkm commented 1 month ago

@rob0rt I'm in the Resty v3 active development. I wanted to check if you got a chance to try the functions above.