Couple of enhancements

[jeevatkm]

Resolves #352
Resolves #370

[codecov]

Codecov Report

Merging #374 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #374   +/-   ##
=======================================
  Coverage   96.19%   96.20%           
=======================================
  Files          10       10           
  Lines        1235     1238    +3     
=======================================
+ Hits         1188     1191    +3     
  Misses         26       26           
  Partials       21       21           

Impacted Files	Coverage Δ
resty.go	100.00% <ø> (ø)
trace.go	100.00% <ø> (ø)
middleware.go	91.54% <100.00%> (ø)
request.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ba00f...7643bc9. Read the comment docs.

[moorereason]

I don't have permission to push to your PR branch, so here's a patch to test the RemoteAddr feature:

diff --git a/request_test.go b/request_test.go
index b02e04c..1035d87 100644
--- a/request_test.go
+++ b/request_test.go
@@ -1644,6 +1644,8 @@ func TestTraceInfo(t *testing.T) {
        ts := createGetServer(t)
        defer ts.Close()

+       serverAddr := ts.URL[strings.LastIndex(ts.URL, "/")+1:]
+
        client := dc()
        client.SetHostURL(ts.URL).EnableTrace()
        for _, u := range []string{"/", "/json", "/long-text", "/long-json"} {
@@ -1660,6 +1662,7 @@ func TestTraceInfo(t *testing.T) {
                assertEqual(t, true, tr.TotalTime >= 0)
                assertEqual(t, true, tr.TotalTime < time.Hour)
                assertEqual(t, true, tr.TotalTime == resp.Time())
+               assertEqual(t, tr.RemoteAddr.String(), serverAddr)
        }

        client.DisableTrace()

[jeevatkm]

Sure, I will add it to the test case.

I don't have permission to push to your PR branch

Hmm, this is strange. You're a member of this repo, you should have write access. PR branch resides in this repo. Can you share the error message?

[jeevatkm]

@moorereason Test case updated

[moorereason]

I see that I'm a "member" of the project, but I appear to only have read access.

sh$ git config --get remote.origin.url
git@github.com:go-resty/resty.git

sh$ git status
On branch few-enhancements
Your branch is ahead of 'origin/few-enhancements' by 1 commit.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

sh$ git push
ERROR: Permission to go-resty/resty.git denied to moorereason.`
fatal: Could not read from remote repository.

[jeevatkm]

@moorereason It's my bad, I created @go-resty/core team but didn't assign it to the repo. I'm sorry. Can you try now?

Also, please review this PR and approve it.

[moorereason]

First, I don't think RetryAttempt needs to be added to the TraceInfo object. Simply exporting it on Request is enough.

Second, can you update the README.md to include RemoteAddr in the TraceInfo example?

Finally, wouldn't it be helpful to set the RetryAttempt for each retry request so that the OnAfter/middleware funcs have access to it? For example...

Given this test program:

package main

import (
        "fmt"
        "net/http"

        "github.com/go-resty/resty/v2"
)

func main() {
        client := resty.New().
                SetRetryCount(3).
                AddRetryCondition(func(r *resty.Response, e error) bool {
                        return r.StatusCode() == http.StatusNotFound
                }).
                OnAfterResponse(func(c *resty.Client, resp *resty.Response) error {
                        fmt.Printf("OnAfterResponse:attempt = %d\n", resp.Request.RetryAttempt)
                        return nil
                })

        resp, _ := client.R().Get("https://httpbin.org/status/404")
        fmt.Printf("%d\n", resp.Request.RetryAttempt)
}

The current PR executes as:

sh$ go run main.go
OnAfterResponse:attempt = 0
OnAfterResponse:attempt = 0
OnAfterResponse:attempt = 0
OnAfterResponse:attempt = 0
4

With this change:

diff --git a/request.go b/request.go
index 9e49fca..51f0520 100644
--- a/request.go
+++ b/request.go
@@ -699,6 +699,7 @@ func (r *Request) Execute(method, url string) (*Response, error) {
                        attempt++

                        r.URL = r.selectAddr(addrs, url, attempt)
+                       r.RetryAttempt = attempt

                        resp, err = r.client.execute(r)
                        if err != nil {

The results become:

sh$ go run main.go
OnAfterResponse:attempt = 1
OnAfterResponse:attempt = 2
OnAfterResponse:attempt = 3
OnAfterResponse:attempt = 4
4

[moorereason]

It looks like the permissions are fixed now. Thanks 👍

[jeevatkm]

@moorereason why do you feel RetryAttempt value is not a candidate for TraceInfo?

Sure, I will update the code and readme for remaining.

[moorereason]

why do you feel RetryAttempt value is not a candidate for TraceInfo?

Because it's already exposed in the Request object. Why copy the value to Request.TraceInfo when it's already on the parent object? Maybe I'm misunderstanding how people are using TraceInfo. Why do you want to add it to TraceInfo?

[jeevatkm]

Typically, trace info or metric values are serialized as JSON sent to metric capture systems like influxdb, prometheus, datadog, newrelic, etc. Currently, resty users are doing this step at their end.

I have been thinking for a long; still, I have not decided field name format snake case vs camel case or support both via function on r.TraceInfo().ToJSON(resty.FieldCamelCase). That's why no traction on this.

[moorereason]

Thanks for the explanation. Makes sense. 👍

I have a couple more comments:

1. We should update README.md to include info about RetryAttempt on the Request and TraceInfo objects.

2. I'm not sure that RetryAttempt is a good property name. If no retry scenario occurred (the request happened the first time without issue), RetryAttempt would be equal to 1. In my mind, there were no "retry attempts" in that scenario. It would be more descriptive to name the property ConnectionAttempt or ConnAttempt or something. What do you think?

[jeevatkm]

Thanks @moorereason

1. We should update README.md to include info about RetryAttempt on the Request and TraceInfo objects.

Sure, I will add details in the readme 👍.

2. I'm not sure that RetryAttempt is a good property name. If no retry scenario occurred (the request happened the first time without issue), RetryAttempt would be equal to 1. In my mind, there were no "retry attempts" in that scenario. It would be more descriptive to name the property ConnectionAttempt or ConnAttempt or something. What do you think?

Nice, good one. The Connection or Conn word might collide with TCP connection info. How about this?

• In the Request struct let's call it as Attempt -> it will bring the meaning of Request.Attempt
• In the TraceInfo struct let's call it as RequestAttempt -> it will bring the meaning of TraceInfo().RequestAttempt

After your response, I will prepare this PR as final.

[moorereason]

How about this?

• In the Request struct let's call it as Attempt -> it will bring the meaning of Request.Attempt
• In the TraceInfo struct let's call it as RequestAttempt -> it will bring the meaning of TraceInfo().RequestAttempt

Sounds good. 👍

[jeevatkm]

@moorereason I have updated it. This PR is ready for final review.

[moorereason]

Small rewording suggestion, but I won't hold up the PR for it. Nice work. 👍

[moorereason]

Rewording suggestion:

Since v2.4.0, trace info contains a RequestAttempt value, and the Request object contains an Attempt attribute

[jeevatkm]

Thanks, @moorereason, I will update the readme with suggestion on the next round. I have to do a few other things.