Improvements for jaegertracing #62
Conversation
Changes: - fixed http.status_code value. Now it is always correct. Ugly workaround removed. - middleware code more readable. - simplified ResponseDumper version instead of bodyDumpResponseWriter - added tags for client real IP and X-Request-ID header - replace span.SetTag with span.LogKV for values with big cardinality.
jaegertracing/jaegertracing.go
Outdated
| } | ||
|
|
||
| // echo request_id back in response | ||
| c.Response().Header().Set(echo.HeaderXRequestID, requestID) |
There was a problem hiding this comment.
This is actually something that the RequestID middleware handles.
Is there es specific reason it is required for jaegertracing?
There was a problem hiding this comment.
No there is no specific reason for doing it in jaegertracing.
| @@ -128,82 +129,101 @@ func TraceWithConfig(config TraceConfig) echo.MiddlewareFunc { | |||
|
|
|||
| req := c.Request() | |||
| opname := "HTTP " + req.Method + " URL: " + c.Path() | |||
| realIP := c.RealIP() | |||
| requestID := getRequestID(c) // request-id generated by reverse-proxy | |||
There was a problem hiding this comment.
Is a request ID required for jaeger tracing?
It looks like it would be compatible with the request ID middleware here (same as below)
There was a problem hiding this comment.
Request id logged into opentracing tags below:
sp.SetTag("request_id", requestID)It is much easier than write custom RequestIDHandler for request-id middleware.
Tags are essential for searching opentracing traces later.
| @@ -18,6 +18,7 @@ import ( | |||
| type mockSpan struct { | |||
| tracer opentracing.Tracer | |||
| tags map[string]interface{} | |||
| logs map[string]interface{} | |||
There was a problem hiding this comment.
What's the reason to separate tags and logs?
The name logs seems a little misleading here. Seems like only body and error message are put there currently.
There was a problem hiding this comment.
Opentracing has two separate concepts: tags and logs. In fact the logs are more complicated: it is timestamped packs for key-values pairs but for simplification here it is written as simple map[string]interface{}. Because tags and logs can both have same keys they are separated.
- dump limit for huge request/response bodies - responseDumper is now private
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
==========================================
+ Coverage 55.41% 59.46% +4.05%
==========================================
Files 8 8
Lines 462 671 +209
==========================================
+ Hits 256 399 +143
- Misses 186 251 +65
- Partials 20 21 +1
Continue to review full report at Codecov.
|
|
Thanks @ont for your contribution. |
Changes:
http.status_codevalue. Now it is always correct. Ugly workaround removed.ResponseDumperversion instead ofbodyDumpResponseWriterreal IPandX-Request-IDheaderspan.SetTagwithspan.LogKVfor values with big cardinality (it can be discussed)