Body NewRequest

[mgutijae]

Add body on NewRequest, not nil

[aldas]

Please add tests and explanation what problem and how this change fixes.

[mgutijae]

Currently, one of the parameters that the function has is the body, but this parameter is never used inside the method. Instead, a nil value is pass into NewRequest function. I think this is a bug and just request without body can be handled.

[mgutijae]

No tests are required for this change

[m1x0n]

Good catch @mgutijae. The issue is easy to reproduce. Just create traced request for HTTP POST and try to send it via http.Client. You will propagate only tracing headers but request will definitely miss body.

[aldas]

Could someone wit actual jaeger instance confirm that it works and I'll merge this PR.

[m1x0n]

Could someone wit actual jaeger instance confirm that it works and I'll merge this PR.

Sure. I can check this.

[m1x0n]

@aldas I did a small concept to prove the fix is working: https://github.com/m1x0n/echo-jaeger/blob/master/srv.go
Results:

~ curl -i http://localhost:1337/broken
HTTP/1.1 400 Bad Request
Content-Type: text/plain; charset=UTF-8
Date: Fri, 01 Jul 2022 12:17:11 GMT
Content-Length: 25

Received: Body is missing%

~ curl -i http://localhost:1337/fixed
HTTP/1.1 200 OK
Content-Type: text/plain; charset=UTF-8
Date: Fri, 01 Jul 2022 12:17:21 GMT
Content-Length: 35

Received: Body is OK: Hello jaeger!%

Jaeger UI:

Also can be checked by bytes_in/bytes_out

echo-jaeger-app-1     | {"time":"2022-07-01T12:17:11.984625937Z","id":"","remote_ip":"127.0.0.1","host":"0.0.0.0:1337","method":"POST","uri":"/body","user_agent":"Go-http-client/1.1","status":400,"error":"","latency":56325,"latency_human":"56.325µs","bytes_in":0,"bytes_out":15}
echo-jaeger-app-1     | {"time":"2022-07-01T12:17:11.985125846Z","id":"","remote_ip":"172.18.0.1","host":"localhost:1337","method":"GET","uri":"/broken","user_agent":"curl/7.79.1","status":400,"error":"","latency":1012973,"latency_human":"1.012973ms","bytes_in":0,"bytes_out":25}
echo-jaeger-app-1     | {"time":"2022-07-01T12:17:21.194249831Z","id":"","remote_ip":"127.0.0.1","host":"0.0.0.0:1337","method":"POST","uri":"/body","user_agent":"Go-http-client/1.1","status":200,"error":"","latency":70114,"latency_human":"70.114µs","bytes_in":13,"bytes_out":25}
echo-jaeger-app-1     | {"time":"2022-07-01T12:17:21.19520424Z","id":"","remote_ip":"172.18.0.1","host":"localhost:1337","method":"GET","uri":"/fixed","user_agent":"curl/7.79.1","status":200,"error":"","latency":1641885,"latency_human":"1.641885ms","bytes_in":0,"bytes_out":35}

[m1x0n]

@aldas The issue is still relevant. Are there any other checks we need to add to get fix merged?

[aldas]

Sorry, I totally forgot. It is now merged and I tagged a new release for it v0.13.1

[m1x0n]

Great! Thank you.