-
Notifications
You must be signed in to change notification settings - Fork 296
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
(WIP) FastHTTP Integration #774
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #774 +/- ##
==========================================
Coverage ? 91.93%
==========================================
Files ? 41
Lines ? 2171
Branches ? 0
==========================================
Hits ? 1996
Misses ? 136
Partials ? 39 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
If you're going to wrap the client, then you should provide a wrapped function for each method of the client object, even if they are self referential https://pkg.go.dev/github.com/valyala/fasthttp#Client. Why not provide a wrapper for DoDeadline, Do Redirects.... etc? |
Do(req *fasthttp.Request, resp *fasthttp.Response) error | ||
} | ||
|
||
func Do(client FastHTTPClient, txn *newrelic.Transaction, req *fasthttp.Request, res *fasthttp.Response) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should provide wrappers for all the methods of the Client object if we're going to wrap it. Otherwise, customers will not have a way to wrap them. The question I have though is if its actually necessary to wrap it at all?
ctx.Request.SetRequestURI("/hello") | ||
wrappedHandler(ctx) | ||
|
||
app.ExpectMetrics(t, []internal.WantMetric{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the wrapHandle test also creates and expects an error. Can we do that for the tests for fasthttp as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an example of noticing an error in the fasthttp-server example :) I don't think it should be too difficult to add that into the internal testing as well.
0d57e7c
to
de5ca15
Compare
Over all, this looks really good :) Everything seems to be working as expected and I like the user experience much better of this version |
7f2ecb5
to
ef22a97
Compare
v3/newrelic/segments.go
Outdated
StartTime: txn.StartSegmentNow(), | ||
Request: request, | ||
} | ||
s.secureAgentEvent = secureAgent.SendEvent("OUTBOUND", request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement should be inside an if IsSecurityAgentPresent(){ ... }
conditional
v3/newrelic/segments.go
Outdated
request.Header.Set(key, value) | ||
} | ||
} | ||
secureAgent.DistributedTraceHeaders(request, s.secureAgentEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement should be inside an if IsSecurityAgentPresent(){ ... }
conditional
15e2da3
to
aed3dde
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me now.
* minor fix for complete security disable flag * Create FastHTTP Client Functions * FastHTTP Request Integration * FastHTTP example file * FastHTTP Request Integration * FastHTTP Response file * mod file * update security agent version * supportability metric * Created unit tests and removed extraneous file * Moved FastHTTP to internal instrumentation * Added testing for errors * chore: add logs-in-context example with logrus * chore: move example to specific folder * FastHTTP external segments/Client example * License for Server Example * Added test for external segment/minor fixes * FastHTTP Integration (#774) Added Support For FastHTTP * V3.25.0 Changelog (#781) * V3.25.0 * update version * corrected changelog for 3.25 release * Fixed test not passing * Update segments.go Removed extra function --------- Co-authored-by: aayush-ap <agarg@newrelic.com> Co-authored-by: Steve Willoughby <76975199+nr-swilloughby@users.noreply.github.com> Co-authored-by: Julien Erard <jerard@newrelic.com> Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com> Co-authored-by: Steve Willoughby <swilloughby@newrelic.com>
ctx := &fasthttp.RequestCtx{} | ||
seg := newrelic.StartExternalSegmentFastHTTP(txn, ctx) | ||
defer seg.End() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx
doesn't contain any outbound req attributes. it's just an empty instance of RequestCtx.
We would require outbound req URI and header for generating trace events.
@mirackara
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aayush-ap Is this still the case if this is on the client side? In the regular client example we do the same thing as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mirackara In the regular client example we are passing HTTP request object as an attribute of StartExternalSegment which contain outbound req parameters like URL and all.
but in the case of fastHttp you are passing fast HTTP RequestCtx object as an attribute of StartExternalSegmentFastHTTP
ctx doesn't contain any outbound req attributes. it's just an empty instance of RequestCtx.
example:
req := fasthttp.AcquireRequest()
.....
......
req.SetRequestURI("https://www.google.com/")
ctx := &fasthttp.RequestCtx{}
seg := newrelic.StartExternalSegmentFastHTTP(txn, ctx)
defer seg.End()
err := fasthttp.Do(req, resp)
-
Expected :
{
"name":"External/www.google.com/https",
"scope":"WebTransaction/Go/GET /case1"
} -
Actual :
{
"name":"External/unknown/http/GET",
"scope":"WebTransaction/Go/GET /doRequest"
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the following fix for outbound request
s.secureAgentEvent = secureAgent.SendEvent("OUTBOUND", request) | ||
} | ||
|
||
if request != nil && request.Header != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.outboundHeaders
and DistributedTraceHeaders
should be added to the original request context before executing the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aayush-ap
Looking at this, the StartExternalSegmentFastHTTP function is identical to the StartExternalSegment function. The only difference being is that we convert to a http request. Im a bit confused at this comment because we are adding s.outboundHeaders
and DistributedTraceHeaders
before returning the request context and executing the request.
Unless you are referring to adding s.secureAgentEvent = secureAgent.SendEvent("OUTBOUND", request)
after we add the two headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are converting ctx
fasthttp.RequestCtx to http request and then adding all the required headers.
But that does not reflect the original request object that is used to call an outbound request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the following fix for outbound request
I'll open a bug fix for this. |
} | ||
|
||
func (rw fasthttpWrapperResponse) Header() http.Header { | ||
hdrs := http.Header{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not able to access inbound request response with fasthttpWrapperResponse
wrapper
@mirackara
* Release 3.25.0 (#782) * minor fix for complete security disable flag * Create FastHTTP Client Functions * FastHTTP Request Integration * FastHTTP example file * FastHTTP Request Integration * FastHTTP Response file * mod file * update security agent version * supportability metric * Created unit tests and removed extraneous file * Moved FastHTTP to internal instrumentation * Added testing for errors * chore: add logs-in-context example with logrus * chore: move example to specific folder * FastHTTP external segments/Client example * License for Server Example * Added test for external segment/minor fixes * FastHTTP Integration (#774) Added Support For FastHTTP * V3.25.0 Changelog (#781) * V3.25.0 * update version * corrected changelog for 3.25 release * Fixed test not passing * Update segments.go Removed extra function --------- Co-authored-by: aayush-ap <agarg@newrelic.com> Co-authored-by: Steve Willoughby <76975199+nr-swilloughby@users.noreply.github.com> Co-authored-by: Julien Erard <jerard@newrelic.com> Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com> Co-authored-by: Steve Willoughby <swilloughby@newrelic.com> * fix out of memory issue for req body * Added new config parameter for read request body * update request body buffer * minor fix for dataTruncated * Update readme file * Update csec-go-agent version * Added new wrapper for go-micro stream server * minor fix for GHA * Fix for cpu overhead * backward compatibility * update agent version * minor fix --------- Co-authored-by: Mirac Kara <55501260+mirackara@users.noreply.github.com> Co-authored-by: Steve Willoughby <76975199+nr-swilloughby@users.noreply.github.com> Co-authored-by: Julien Erard <jerard@newrelic.com> Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com> Co-authored-by: Steve Willoughby <swilloughby@newrelic.com>
* Error Expected Bug The attribute error.expected should be a boolean, not a string. It is also good practice to use a constant value for the key. * Bump google.golang.org/grpc from 1.54.0 to 1.56.3 in /v3/integrations/nrgraphqlgo/example (#811) --------- * Bump google.golang.org/grpc in /v3/integrations/nrgraphqlgo/example Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.54.0 to 1.56.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.54.0...v1.56.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> --------- * Bump google.golang.org/grpc from 1.54.0 to 1.56.3 in /v3/integrations/nrgrpc (#810) --------- * Bump google.golang.org/grpc in /v3/integrations/nrgrpc Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.54.0 to 1.56.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.54.0...v1.56.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> --------- * Bump google.golang.org/grpc from 1.54.0 to 1.56.3 in /v3 (#809) --------- * Bump google.golang.org/grpc from 1.54.0 to 1.56.3 in /v3 Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.54.0 to 1.56.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.54.0...v1.56.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> --------- * Bump golang.org/x/net from 0.8.0 to 0.17.0 in /v3/integrations/nrgraphqlgo/example (#804) --------- * Bump golang.org/x/net in /v3/integrations/nrgraphqlgo/example Bumps [golang.org/x/net](https://github.com/golang/net) from 0.8.0 to 0.17.0. - [Commits](golang/net@v0.8.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> --------- * Fix for out of memory error with request body (#806) * Release 3.25.0 (#782) * minor fix for complete security disable flag * Create FastHTTP Client Functions * FastHTTP Request Integration * FastHTTP example file * FastHTTP Request Integration * FastHTTP Response file * mod file * update security agent version * supportability metric * Created unit tests and removed extraneous file * Moved FastHTTP to internal instrumentation * Added testing for errors * chore: add logs-in-context example with logrus * chore: move example to specific folder * FastHTTP external segments/Client example * License for Server Example * Added test for external segment/minor fixes * FastHTTP Integration (#774) Added Support For FastHTTP * V3.25.0 Changelog (#781) * V3.25.0 * update version * corrected changelog for 3.25 release * Fixed test not passing * Update segments.go Removed extra function --------- Co-authored-by: aayush-ap <agarg@newrelic.com> Co-authored-by: Steve Willoughby <76975199+nr-swilloughby@users.noreply.github.com> Co-authored-by: Julien Erard <jerard@newrelic.com> Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com> Co-authored-by: Steve Willoughby <swilloughby@newrelic.com> * fix out of memory issue for req body * Added new config parameter for read request body * update request body buffer * minor fix for dataTruncated * Update readme file * Update csec-go-agent version * Added new wrapper for go-micro stream server * minor fix for GHA * Fix for cpu overhead * backward compatibility * update agent version * minor fix --------- Co-authored-by: Mirac Kara <55501260+mirackara@users.noreply.github.com> Co-authored-by: Steve Willoughby <76975199+nr-swilloughby@users.noreply.github.com> Co-authored-by: Julien Erard <jerard@newrelic.com> Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com> Co-authored-by: Steve Willoughby <swilloughby@newrelic.com> * move fasthttp out of core library, and into integration package (#808) * move fasthttp out of core library, and into integration package * move examples over * add security agent headers to fasthttp object * fix examples and external segment * add fasthttp tests * cleanup of go mods * fix segment collection * add security agent inbound write capture to wrapped handle func * Update go.mod * Update Changelog * update version.go --------- Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: aayush-ap <59004877+aayush-ap@users.noreply.github.com> Co-authored-by: Mirac Kara <55501260+mirackara@users.noreply.github.com> Co-authored-by: Steve Willoughby <76975199+nr-swilloughby@users.noreply.github.com> Co-authored-by: Julien Erard <jerard@newrelic.com> Co-authored-by: Steve Willoughby <swilloughby@newrelic.com> Co-authored-by: mirackara <mirackara2000@outlook.com>
No description provided.