newrelic / go-agent

New Relic Go Agent
Apache License 2.0
762 stars 294 forks source link

more robust solution for obfuscation #828

Closed iamemilio closed 9 months ago

iamemilio commented 9 months ago

Links

Details

codecov-commenter commented 9 months ago

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (7b07e07) 80.03% compared to head (4f77a5a) 79.70%. Report is 3 commits behind head on develop.

Files Patch % Lines
v3/newrelic/internal_app.go 11.11% 8 Missing :warning:
v3/newrelic/collector.go 92.30% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #828 +/- ## =========================================== - Coverage 80.03% 79.70% -0.34% =========================================== Files 136 122 -14 Lines 12358 11486 -872 =========================================== - Hits 9891 9155 -736 + Misses 2183 2065 -118 + Partials 284 266 -18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

A-deLuna commented 9 months ago

We are also seeing the error logged here:

        if nil != resp.Err {
            app.Warn("application connect failure", map[string]interface{}{
                "error": resp.Err.Error(),
            })
        }

I skimmed through the PRs and I think this part may not be covered yet.

A-deLuna commented 9 months ago

Hope you don't mind me chiming in.

I think the issue originates from this line:

    resp, err := cs.Client.Do(req)
    if err != nil {
        return rpmResponse{
            forceSaveHarvestData: true,
            Err:                  err,
        }
    }

http.Client.Do always returns *url.Errors:

Any returned error will be of type *url.Error

And url.Error.Error() always prints the URL in it's string representation.

func (e *Error) Error() string { return fmt.Sprintf("%s %q: %s", e.Op, e.URL, e.Err) }

I think the part we want to obfuscate is the URL.

We could obfuscate by checking if the error is of type *url.Error. And maybe clear out the URL, or cast the error type to a defined type that has a different implementation of Error() string, or whatever else you think is best.

iamemilio commented 9 months ago

Hope you don't mind me chiming in.

I think the issue originates from this line:

  resp, err := cs.Client.Do(req)
  if err != nil {
      return rpmResponse{
          forceSaveHarvestData: true,
          Err:                  err,
      }
  }

http.Client.Do always returns *url.Errors:

Any returned error will be of type *url.Error

And url.Error.Error() always prints the URL in it's string representation.

func (e *Error) Error() string { return fmt.Sprintf("%s %q: %s", e.Op, e.URL, e.Err) }

I think the part we want to obfuscate is the URL.

We could obfuscate by checking if the error is of type *url.Error. And maybe clear out the URL, or cast the error type to a defined type that has a different implementation of Error() string, or whatever else you think is best.

that's a very good idea, let me take a look. I am also going to check if we are printing URLs for other ingestion related errors, because this issue is probably not a one off.

iamemilio commented 9 months ago

Yes, feel free to squash this.