Skip to content
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.

Sign up for GitHub

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

Add "isDecodable" option to middleware function call #141

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Add "isDecodable" option to middleware function call #141

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6737d67
Add bool param to middleware for decoding
renaz6 Jun 5, 2023
04301ea
Merge branch 'main' into tr1d1umCandlelightBug
renaz6 Jun 5, 2023
9da3452
Add Check for traceheader, ensure tracing
renaz6 Jun 13, 2023
f7ba5dc
Merge branch 'tr1d1umCandlelightBug' of https://github.com/xmidt-org/…
renaz6 Jun 13, 2023
8bc6dc3
Correct Xmidt header
renaz6 Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package candlelight

import (
"context"
"crypto/rand"
"encoding/base64"
"net/http"
Expand Down Expand Up @@ -62,34 +63,42 @@ func (traceConfig *TraceConfig) TraceMiddleware(delegate http.Handler) http.Hand
})
}

// EchoFirstNodeTraceInfo captures the trace information from a request and writes it
// back in the response headers if the request is the first one in the trace path.
func EchoFirstTraceNodeInfo(propagator propagation.TextMapPropagator) func(http.Handler) http.Handler {
// EchoFirstNodeTraceInfo captures the trace information from a request, writes it
// back in the response headers, and adds it to the request's context
// It can also decode the request and save the resulting WRP object in the context if isDecodable is true
func EchoFirstTraceNodeInfo(propagator propagation.TextMapPropagator, isDecodable bool) func(http.Handler) http.Handler {
Copy link
Contributor

Choose a reason for hiding this comment

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

For future:

This is the kind of function that will probably grow over time, with new parameters. This makes the API brittle, since you wind up breaking existing clients when you change the function's signature.

If we add more parameters here, it would probably be better to move to either the builder pattern or the functional options pattern described by https://golang.cafe/blog/golang-functional-options-pattern.html.

An really trivial example:

type TraceOption func(*traceHandlerBuilder)

type traceHandlerBuilder {
    // whatever fields are necessary to build the handler
}

func (builder traceHandlerBuilder) build() func(http.Handler) http.Handler { ... }

func EchoFirstTraceNodeInfo(opts ...TraceOption) func(http.Handler) http.Handler {
   var builder traceHandlerBuilder
   for _, o := range opts {
        o(&builder)
   }

   return o.build()
}

Alternatively, you can expose the builder as part of your API. gorilla/mux takes this approach. The choice of which is really more about what will be more stable for a client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a ticket for this work: #142

return func(delegate http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

if req, err := wrphttp.DecodeRequest(r, nil); err == nil {
r = req
}
var ctx context.Context

var traceHeaders []string
ctx := propagator.Extract(r.Context(), propagation.HeaderCarrier(r.Header))
if msg, ok := wrpcontext.Get[*wrp.Message](ctx); ok {
traceHeaders = msg.Headers
}
if isDecodable {
if req, err := wrphttp.DecodeRequest(r, nil); err == nil {
r = req
}
denopink marked this conversation as resolved.
Show resolved Hide resolved

// Iterate through the trace headers (if any), format them, and add them to ctx
var tmp propagation.TextMapCarrier = propagation.MapCarrier{}
for _, f := range traceHeaders {
if f != "" {
parts := strings.Split(f, ":")
// Remove leading space if there's any
parts[1] = strings.Trim(parts[1], " ")
tmp.Set(parts[0], parts[1])
var traceHeaders []string
ctx = propagator.Extract(r.Context(), propagation.HeaderCarrier(r.Header))
if msg, ok := wrpcontext.Get[*wrp.Message](ctx); ok {
traceHeaders = msg.Headers
}

// Iterate through the trace headers (if any), format them, and add them to ctx
var tmp propagation.TextMapCarrier = propagation.MapCarrier{}
for _, f := range traceHeaders {
if f != "" {
parts := strings.Split(f, ":")
// Remove leading space if there's any
parts[1] = strings.Trim(parts[1], " ")
tmp.Set(parts[0], parts[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to check whether or not parts is at least len of 2 in case someone gives us a "bad" trace header (if that's possible)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

}
}

ctx = propagation.TraceContext{}.Extract(ctx, tmp)
} else {
ctx = propagator.Extract(r.Context(), propagation.HeaderCarrier(r.Header))
}

ctx = propagation.TraceContext{}.Extract(ctx, tmp)
sc := trace.SpanContextFromContext(ctx)
if sc.IsValid() {
w.Header().Set(spanIDHeaderName, sc.SpanID().String())
Expand Down