Skip to content

Commit

Permalink
Added fix for fasthttp outbound request
Browse files Browse the repository at this point in the history
  • Loading branch information
@aayush-ap
aayush-ap committed Oct 13, 2023
1 parent d0a46d0 commit 15a72c8
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 10 deletions.
2 changes: 1 addition & 1 deletion v3/examples/client-fasthttp/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func doRequest(txn *newrelic.Transaction) error {
req.Header.SetMethod("GET")

ctx := &fasthttp.RequestCtx{}
seg := newrelic.StartExternalSegmentFastHTTP(txn, ctx)
seg := newrelic.StartExternalSegmentFastHTTP(txn, ctx, req)
defer seg.End()

err := fasthttp.Do(req, resp)
Expand Down
10 changes: 7 additions & 3 deletions v3/newrelic/context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,15 @@ func transactionFromRequestContext(req *http.Request) *Transaction {
return txn
}

func transactionFromRequestContextFastHTTP(ctx *fasthttp.RequestCtx) *Transaction {
func transactionFromRequestContextFastHTTP(ctx interface{}) *Transaction {

This comment has been minimized.

Sign in to view

Copy link
@iamemilio

iamemilio Oct 31, 2023 •
edited

@aayush-ap why do we need this? If the Transaction is in a regular context, we can just use newrelic.FromContext()

This comment has been minimized.

Sign in to view

Copy link
@aayush-ap

aayush-ap Nov 1, 2023

Author

@iamemilio
No this is not a regular context always
It can be context.Context or fasthttp.RequestCtx.

This comment has been minimized.

Sign in to view

Copy link
@iamemilio

iamemilio Nov 2, 2023

does Fasthttp use context.Context? Why can the customer not just use the existing newrelic.FromContext() function to get the transaction out of a context.Context?

This comment has been minimized.

Sign in to view

Copy link
@aayush-ap

aayush-ap Nov 3, 2023

Author

yes, customers can get the transaction out of a context.Context
I just added handling so that customers don't need to add extra lines to get transactions from context.Context
var txn *Transaction
if nil != ctx {
txn := ctx.UserValue("transaction").(*Transaction)
return txn
switch context := ctx.(type) {
case *fasthttp.RequestCtx:
txn = context.UserValue("transaction").(*Transaction)
case context.Context:
txn = FromContext(context)
}
}

if txn != nil {
Expand Down
2 changes: 1 addition & 1 deletion v3/newrelic/internal_context_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestExternalSegmentFastHTTP(t *testing.T) {
req.Header.SetMethod("GET")

ctx := &fasthttp.RequestCtx{}
seg := StartExternalSegmentFastHTTP(txn, ctx)
seg := StartExternalSegmentFastHTTP(txn, ctx, req)
defer seg.End()

txn.End()
Expand Down
17 changes: 12 additions & 5 deletions v3/newrelic/segments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,13 +340,14 @@ func StartExternalSegment(txn *Transaction, request *http.Request) *ExternalSegm
return s
}

func StartExternalSegmentFastHTTP(txn *Transaction, ctx *fasthttp.RequestCtx) *ExternalSegment {
func StartExternalSegmentFastHTTP(txn *Transaction, ctx interface{}, req *fasthttp.Request) *ExternalSegment {

This comment has been minimized.

Sign in to view

Copy link
@iamemilio

iamemilio Oct 31, 2023

I don't understand why we need an interface and the fastHttp.Request objects here.

This comment has been minimized.

Sign in to view

Copy link
@aayush-ap

aayush-ap Nov 1, 2023

Author

@iamemilio

  1. We need ctx as interface because ctx can be context.Context or fasthttp.RequestCtx

Please find the attached example

case1.txt
case2.txt

  1. We need fastHttp.Request objects to append outbound header in original req object. with fasthttp.RequestCtx this is not possible

This comment has been minimized.

Sign in to view

Copy link
@iamemilio

iamemilio Nov 2, 2023

Case 1 is not using Fasthttp, which is why it has a context.Context. I don't think it makes sense to use both http and Fasthttp like that. In case 1, customers can just use the tooling that already exists for http.

This comment has been minimized.

Sign in to view

Copy link
@aayush-ap

aayush-ap Nov 3, 2023

Author

okay if customers will do this we don't need to add extra handling for context.Context.
if nil == txn {
txn = transactionFromRequestContextFastHTTP(ctx)
}
request := &http.Request{}

fasthttpadaptor.ConvertRequest(ctx, request, true)
dummy_ctx := &fasthttp.RequestCtx{Request: *req}
request := &http.Request{}
fasthttpadaptor.ConvertRequest(dummy_ctx, request, true)
s := &ExternalSegment{
StartTime: txn.StartSegmentNow(),
Request: request,
Expand All @@ -355,15 +356,21 @@ func StartExternalSegmentFastHTTP(txn *Transaction, ctx *fasthttp.RequestCtx) *E
s.secureAgentEvent = secureAgent.SendEvent("OUTBOUND", request)
}

if request != nil && request.Header != nil {
request = &http.Request{}
if req != nil {
for key, values := range s.outboundHeaders() {
for _, value := range values {
request.Header.Set(key, value)
req.Header.Set(key, value)
}
}

if IsSecurityAgentPresent() {
secureAgent.DistributedTraceHeaders(request, s.secureAgentEvent)
for key, values := range request.Header {
for _, value := range values {
req.Header.Set(key, value)
}
}
}
}

Expand Down

2 comments on commit 15a72c8

@iamemilio
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aayush-ap this only needs to serve Fasthttp. We have also been asked to move this code into its own extension for security reasons: https://github.com/newrelic/go-agent/pull/808/files

@iamemilio
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us know if the k2 agent needs any changes from this PR

Please sign in to comment.