SMQ-2671 - Add request ID and use it to correlate traces, logs, and events #2709
Conversation
nyagamunene
force-pushed
the
SMQ-2671
branch
2 times, most recently
from
d3a4444 to
ecc6904
Compare
nyagamunene
force-pushed
the
SMQ-2671
branch
from
8b13f92 to
5586cf5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2709 +/- ##
==========================================
- Coverage 42.08% 37.70% -4.39%
==========================================
Files 341 209 -132
Lines 47488 36425 -11063
==========================================
- Hits 19987 13734 -6253
+ Misses 25318 21618 -3700
+ Partials 2183 1073 -1110 ☔ View full report in Codecov by Sentry. |
api/http/authn.go
Outdated
| @@ -47,3 +49,20 @@ func AuthenticateMiddleware(authn smqauthn.Authentication, domainCheck bool) fun | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func RequestIDMiddleware() func(http.Handler) http.Handler { | |||
There was a problem hiding this comment.
This should be in utils.
api/http/authn.go
Outdated
| func RequestIDMiddleware() func(http.Handler) http.Handler { | ||
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| idp := uuid.New() |
There was a problem hiding this comment.
We should not instantiate a new provider in the handler method, this will result in a new provider for each request.
certs/api/transport.go
Outdated
| @@ -41,6 +41,8 @@ func MakeHandler(svc certs.Service, authn smqauthn.Authentication, logger *slog. | |||
|
|
|||
| r.Group(func(r chi.Router) { | |||
| r.Use(api.AuthenticateMiddleware(authn, true)) | |||
| r.Use(api.RequestIDMiddleware()) | |||
There was a problem hiding this comment.
Did you consider using Chi native https://github.com/go-chi/chi/blob/master/middleware/request_id.go? Is the key format the main reason we're not using it?
There was a problem hiding this comment.
Did you consider using Chi native https://github.com/go-chi/chi/blob/master/middleware/request_id.go? Is the key format the main reason we're not using it?
The issue is the format which looks like this 1d825ef43136/dFDftFPuGC-000001 which will be challenge to convert to a traceID
certs/tracing/tracing.go
Outdated
| @@ -23,9 +31,28 @@ func New(svc certs.Service, tracer trace.Tracer) certs.Service { | |||
| return &tracingMiddleware{tracer, svc} | |||
| } | |||
|
|
|||
| func (tm *tracingMiddleware) startSpan(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { | |||
There was a problem hiding this comment.
Can we extract this so we don't repeat it all over?
certs/tracing/tracing.go
Outdated
| reqID := middleware.GetReqID(ctx) | ||
| if reqID != "" { | ||
| cleanID := strings.ReplaceAll(reqID, separator, emptyString) | ||
| final := fmt.Sprintf("%032s", cleanID) |
There was a problem hiding this comment.
Use constant for the format, just like for separator and empty string.
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
dborovcanin
force-pushed
the
SMQ-2671
branch
from
81077b2 to
073aa3c
Compare
What type of PR is this?
This is a feature because it adds the following functionality:
What does this do?
It adds request ID and uses it to correlate trace, logs and events.
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
No.
Did you document any new/modified feature?
No.
Notes