Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Middleware around resty.Request.Execute including retries #788

Open
rob0rt opened this issue Apr 10, 2024 · 1 comment
Open

Middleware around resty.Request.Execute including retries #788

rob0rt opened this issue Apr 10, 2024 · 1 comment

Comments

@rob0rt
Copy link

rob0rt commented Apr 10, 2024

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.

@rob0rt rob0rt changed the title Middleware around request + retries Middleware around all requests made with *resty.Request including retries Apr 10, 2024
@rob0rt rob0rt changed the title Middleware around all requests made with *resty.Request including retries Middleware around resty.Request.Execute including retries Apr 10, 2024
@jeevatkm
Copy link
Member

jeevatkm commented May 9, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants