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

Jump to bottom

[hotROD] Upgrade hotrod to support otel tracer #4548

Merged
merged 23 commits into from
Jul 15, 2023
Merged

[hotROD] Upgrade hotrod to support otel tracer #4548

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9a49fab
adds unit test for driver.service server
afzal442 Jun 24, 2023
bf4044f
comments hardcoded response
afzal442 Jun 24, 2023
c8b3136
fixes linting issues
afzal442 Jun 24, 2023
f93973e
refactors ot to otel trace
afzal442 Jun 26, 2023
a9a576a
adds otelhttp to driver.client
afzal442 Jun 27, 2023
960a203
adds otel tracer to mysql service
afzal442 Jun 28, 2023
ca97d72
reformats functions call
afzal442 Jun 30, 2023
57c5864
refactors server mux
afzal442 Jun 30, 2023
945a5f3
reformats HTTPClient
afzal442 Jun 30, 2023
0986d24
adds span context from trace
afzal442 Jul 3, 2023
6132dc6
rmvs otel propagator
afzal442 Jul 4, 2023
a32768e
rmvs baggage items
afzal442 Jul 7, 2023
ef1eb6b
cleanup and put the http client in order
afzalbin64 Jul 10, 2023
48b60c1
makes http getJson to use Client instance
afzalbin64 Jul 10, 2023
37357a5
adds return obj to handler
afzal442 Jul 10, 2023
3baa22a
minor fixes and adds otel baggage syntax
afzal442 Jul 10, 2023
5249338
Merge remote-tracking branch 'upstream/main' into upgrade-hotrod-to-otel
afzal442 Jul 10, 2023
189e3df
adds span attributes
afzal442 Jul 10, 2023
bc505ba
adds baggage member with context
afzalbin64 Jul 11, 2023
40165c4
rms new bag creation
afzalbin64 Jul 11, 2023
b93c17f
Merge remote-tracking branch 'upstream/main' into upgrade-hotrod-to-otel
afzalbin64 Jul 11, 2023
69da401
adds logger to log the baggage issue
afzal442 Jul 12, 2023
5dd40a9
substitute customer nm with `_`
afzal442 Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/hotrod/cmd/frontend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ var frontendCmd = &cobra.Command{
logger := log.NewFactory(zapLogger)
server := frontend.NewServer(
options,
tracing.Init("frontend", otelExporter, metricsFactory, logger),
tracing.InitOTEL("frontend", otelExporter, metricsFactory, logger),
afzal442 marked this conversation as resolved.
Show resolved Hide resolved
logger,
)
return logError(zapLogger, server.Run())
Expand Down
2 changes: 1 addition & 1 deletion examples/hotrod/cmd/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var routeCmd = &cobra.Command{
logger := log.NewFactory(zapLogger)
server := route.NewServer(
net.JoinHostPort("0.0.0.0", strconv.Itoa(routePort)),
tracing.Init("route", otelExporter, metricsFactory, logger),
tracing.InitOTEL("route", otelExporter, metricsFactory, logger),
logger,
)
return logError(zapLogger, server.Run())
Expand Down
26 changes: 18 additions & 8 deletions examples/hotrod/pkg/tracing/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,37 @@
"io"
"net/http"

"github.com/opentracing-contrib/go-stdlib/nethttp"
"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
"go.opentelemetry.io/otel/trace"
)

// HTTPClient wraps an http.Client with tracing instrumentation.
type HTTPClient struct {
Tracer opentracing.Tracer
Client *http.Client
TracerProvider trace.TracerProvider
Client *http.Client
}

func NewHTTPClient(tp trace.TracerProvider) *HTTPClient {
return &HTTPClient{
TracerProvider: tp,
Client: &http.Client{
Transport: otelhttp.NewTransport(
http.DefaultTransport,
otelhttp.WithTracerProvider(tp),
),
},
}
}

// GetJSON executes HTTP GET against specified url and tried to parse
// the response into out object.
func (c *HTTPClient) GetJSON(ctx context.Context, endpoint string, url string, out interface{}) error {
req, err := http.NewRequest(http.MethodGet, url, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return err
}
req = req.WithContext(ctx)
req, ht := nethttp.TraceRequest(c.Tracer, req, nethttp.OperationName("HTTP GET: "+endpoint))
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
defer ht.Finish()

res, err := c.Client.Do(req)

Check failure

Code scanning / CodeQL

Uncontrolled data used in network request Critical

The
URL
of this request depends on a
user-provided value
.
if err != nil {
return err
}
Expand All @@ -57,6 +66,7 @@
}
return errors.New(string(body))
}

decoder := json.NewDecoder(res.Body)
return decoder.Decode(out)
}
50 changes: 10 additions & 40 deletions examples/hotrod/pkg/tracing/mux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,15 @@ package tracing
import (
"net/http"

"github.com/opentracing-contrib/go-stdlib/nethttp"
"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/otel/baggage"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/log"
)

// NewServeMux creates a new TracedServeMux.
func NewServeMux(copyBaggage bool, tracer opentracing.Tracer, logger log.Factory) *TracedServeMux {
func NewServeMux(copyBaggage bool, tracer trace.TracerProvider, logger log.Factory) *TracedServeMux {
return &TracedServeMux{
mux: http.NewServeMux(),
copyBaggage: copyBaggage,
Expand All @@ -41,51 +39,23 @@ func NewServeMux(copyBaggage bool, tracer opentracing.Tracer, logger log.Factory
type TracedServeMux struct {
mux *http.ServeMux
copyBaggage bool
tracer opentracing.Tracer
tracer trace.TracerProvider
logger log.Factory
}

// Handle implements http.ServeMux#Handle, which is used to register new handler.
func (tm *TracedServeMux) Handle(pattern string, handler http.Handler) {
tm.logger.Bg().Debug("registering traced handler", zap.String("endpoint", pattern))

middleware := nethttp.Middleware(
tm.tracer,
handler,
nethttp.OperationNameFunc(func(r *http.Request) string {
return "HTTP " + r.Method + " " + pattern
}),
// Jaeger SDK was able to accept `jaeger-baggage` header even for requests without am active trace.
// OTEL Bridge does not support that, so we use Baggage propagator to manually extract the baggage
// into Context (in otelBaggageExtractor handler below), and once the Bridge creates a Span,
// we use this SpanObserver to copy OTEL baggage from Context into the Span.
nethttp.MWSpanObserver(func(span opentracing.Span, r *http.Request) {
if !tm.copyBaggage {
afzal442 marked this conversation as resolved.
Show resolved Hide resolved
return
}
bag := baggage.FromContext(r.Context())
for _, m := range bag.Members() {
if b := span.BaggageItem(m.Key()); b == "" {
span.SetBaggageItem(m.Key(), m.Value())
}
}
}),
)
tm.mux.Handle(pattern, otelBaggageExtractor(middleware))
middleware := otelhttp.NewHandler(
otelhttp.WithRouteTag(pattern, handler),
pattern,
otelhttp.WithTracerProvider(tm.tracer))

tm.mux.Handle(pattern, middleware)
afzal442 marked this conversation as resolved.
Show resolved Hide resolved
}

// ServeHTTP implements http.ServeMux#ServeHTTP.
func (tm *TracedServeMux) ServeHTTP(w http.ResponseWriter, r *http.Request) {
tm.mux.ServeHTTP(w, r)
}

// Used with nethttp.MWSpanObserver above.
func otelBaggageExtractor(next http.Handler) http.Handler {
propagator := propagation.Baggage{}
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
carrier := propagation.HeaderCarrier(r.Header)
ctx := propagator.Extract(r.Context(), carrier)
r = r.WithContext(ctx)
next.ServeHTTP(w, r)
})
}
15 changes: 4 additions & 11 deletions examples/hotrod/services/customer/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ package customer
import (
"context"
"fmt"
"net/http"

"github.com/opentracing-contrib/go-stdlib/nethttp"
"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/log"
Expand All @@ -30,21 +28,16 @@ import (

// Client is a remote client that implements customer.Interface
type Client struct {
tracer opentracing.Tracer
logger log.Factory
client *tracing.HTTPClient
hostPort string
}

// NewClient creates a new customer.Client
func NewClient(tracer opentracing.Tracer, logger log.Factory, hostPort string) *Client {
func NewClient(tracer trace.TracerProvider, logger log.Factory, hostPort string) *Client {
return &Client{
tracer: tracer,
logger: logger,
client: &tracing.HTTPClient{
Client: &http.Client{Transport: &nethttp.Transport{}},
Tracer: tracer,
},
logger: logger,
client: tracing.NewHTTPClient(tracer),
hostPort: hostPort,
}
}
Expand Down
6 changes: 3 additions & 3 deletions examples/hotrod/services/customer/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"encoding/json"
"net/http"

"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/httperr"
Expand All @@ -31,7 +31,7 @@ import (
// Server implements Customer service
type Server struct {
hostPort string
tracer opentracing.Tracer
tracer trace.TracerProvider
logger log.Factory
database *database
}
Expand All @@ -40,7 +40,7 @@ type Server struct {
func NewServer(hostPort string, otelExporter string, metricsFactory metrics.Factory, logger log.Factory) *Server {
return &Server{
hostPort: hostPort,
tracer: tracing.Init("customer", otelExporter, metricsFactory, logger),
tracer: tracing.InitOTEL("customer", otelExporter, metricsFactory, logger),
logger: logger,
database: newDatabase(
tracing.InitOTEL("mysql", otelExporter, metricsFactory, logger).Tracer("mysql"),
Expand Down
12 changes: 5 additions & 7 deletions examples/hotrod/services/driver/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import (
"context"
"time"

otgrpc "github.com/opentracing-contrib/go-grpc"
"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
Expand All @@ -30,25 +30,23 @@ import (

// Client is a remote client that implements driver.Interface
type Client struct {
tracer opentracing.Tracer
logger log.Factory
client DriverServiceClient
}

// NewClient creates a new driver.Client
func NewClient(tracer opentracing.Tracer, logger log.Factory, hostPort string) *Client {
func NewClient(tracerProvider trace.TracerProvider, logger log.Factory, hostPort string) *Client {
conn, err := grpc.Dial(hostPort, grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithUnaryInterceptor(
otgrpc.OpenTracingClientInterceptor(tracer)),
otelgrpc.UnaryClientInterceptor(otelgrpc.WithTracerProvider(tracerProvider))),
grpc.WithStreamInterceptor(
otgrpc.OpenTracingStreamClientInterceptor(tracer)))
otelgrpc.StreamClientInterceptor(otelgrpc.WithTracerProvider(tracerProvider))))
afzal442 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
logger.Bg().Fatal("Cannot create gRPC connection", zap.Error(err))
}

client := NewDriverServiceClient(conn)
return &Client{
tracer: tracer,
logger: logger,
client: client,
}
Expand Down
12 changes: 7 additions & 5 deletions examples/hotrod/services/frontend/best_eta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import (
"sync"
"time"

"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/otel/baggage"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/log"
Expand All @@ -47,7 +48,7 @@ type Response struct {
ETA time.Duration
}

func newBestETA(tracer opentracing.Tracer, logger log.Factory, options ConfigOptions) *bestETA {
func newBestETA(tracer trace.TracerProvider, logger log.Factory, options ConfigOptions) *bestETA {
return &bestETA{
customer: customer.NewClient(
tracer,
Expand Down Expand Up @@ -76,9 +77,10 @@ func (eta *bestETA) Get(ctx context.Context, customerID string) (*Response, erro
}
eta.logger.For(ctx).Info("Found customer", zap.Any("customer", customer))

if span := opentracing.SpanFromContext(ctx); span != nil {
span.SetBaggageItem("customer", customer.Name)
}
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
m, _ := baggage.NewMember("customer", customer.Name)
bag := baggage.FromContext(ctx)
bag, _ = bag.SetMember(m)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
m, _ := baggage.NewMember("customer", customer.Name)
bag := baggage.FromContext(ctx)
bag, _ = bag.SetMember(m)
m, err := baggage.NewMember("customer", customer.Name)
if err != nil {
eta.logger.For(ctx).Error("cannot create baggage member", zap.Error(err))
}
bag := baggage.FromContext(ctx)
bag, err = bag.SetMember(m)
if err != nil {
eta.logger.For(ctx).Error("cannot set baggage member", zap.Error(err))
}

Copy link
Member

Choose a reason for hiding this comment

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

this change will log the errors. We're hitting a bug in OTEL, either the same as open-telemetry/opentelemetry-go#3685 or related. Basically, NewMember requires the value to be escaped, which is an incorrect behavior. But when I tried escaping it with url.EscapeQuery, then the baggage later is not serialized / deserialized at all.

Copy link
Contributor Author

@afzal442 afzal442 Jul 12, 2023 •
edited

Choose a reason for hiding this comment

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

Cool! ignoring err cost me here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro after deep investigation, it came out working with the response in the HTTP request. e.g. Loading customer {"service": "customer", "component": "mysql", "trace_id": "63718f14ff8d04607e60d95da54784ae", "span_id": "b60a6202006008bc", "customer_id": "567"} here, when I try to use any of the items included say "span_id" or customer.ID or customerID or "mysql", I am getting baggage items out of it. Make sure the service should be "customer".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most insane. when I try to add logs into it, it's still not working. INFO customer/database.go:86 Getting customer {"service": "customer", "component": "mysql", "trace_id": "49af5501b6c5c75acfb1e69d19f1e481", "span_id": "e9e53f530cb6aacc", "customer_name": "Amazing Coffee Roasters"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after making minor changes, I got sth like below

"route.calc.by.customer.sec": {"Amazing_Coffee_Roasters": 0.498, "Japanese_Desserts": 0.486},
"route.calc.by.session.sec": {"1581": 0.498, "7406": 0.486}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's the bug it OTEL - users are not supposed to mangle the values like this in order to put them into baggage.

ctx = baggage.ContextWithBaggage(ctx, bag)
afzal442 marked this conversation as resolved.
Show resolved Hide resolved

drivers, err := eta.driver.FindNearest(ctx, customer.Location)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions examples/hotrod/services/frontend/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"net/http"
"path"

"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/httperr"
Expand All @@ -36,7 +36,7 @@ var assetFS embed.FS
// Server implements jaeger-demo-frontend service
type Server struct {
hostPort string
tracer opentracing.Tracer
tracer trace.TracerProvider
logger log.Factory
bestETA *bestETA
assetFS http.FileSystem
Expand All @@ -56,7 +56,7 @@ type ConfigOptions struct {
}

// NewServer creates a new frontend.Server
func NewServer(options ConfigOptions, tracer opentracing.Tracer, logger log.Factory) *Server {
func NewServer(options ConfigOptions, tracer trace.TracerProvider, logger log.Factory) *Server {
return &Server{
hostPort: options.FrontendHostPort,
tracer: tracer,
Expand Down
15 changes: 4 additions & 11 deletions examples/hotrod/services/route/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@ package route

import (
"context"
"net/http"
"net/url"

"github.com/opentracing-contrib/go-stdlib/nethttp"
"github.com/opentracing/opentracing-go"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/log"
Expand All @@ -30,21 +28,16 @@ import (

// Client is a remote client that implements route.Interface
type Client struct {
tracer opentracing.Tracer
logger log.Factory
client *tracing.HTTPClient
hostPort string
}

// NewClient creates a new route.Client
func NewClient(tracer opentracing.Tracer, logger log.Factory, hostPort string) *Client {
func NewClient(tracer trace.TracerProvider, logger log.Factory, hostPort string) *Client {
return &Client{
tracer: tracer,
logger: logger,
client: &tracing.HTTPClient{
Client: &http.Client{Transport: &nethttp.Transport{}},
Tracer: tracer,
},
logger: logger,
client: tracing.NewHTTPClient(tracer),
hostPort: hostPort,
}
}
Expand Down
6 changes: 3 additions & 3 deletions examples/hotrod/services/route/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ import (
"net/http"
"time"

"github.com/opentracing/opentracing-go"
"github.com/prometheus/client_golang/prometheus/promhttp"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/examples/hotrod/pkg/delay"
Expand All @@ -38,12 +38,12 @@ import (
// Server implements Route service
type Server struct {
hostPort string
tracer opentracing.Tracer
tracer trace.TracerProvider
logger log.Factory
}

// NewServer creates a new route.Server
func NewServer(hostPort string, tracer opentracing.Tracer, logger log.Factory) *Server {
func NewServer(hostPort string, tracer trace.TracerProvider, logger log.Factory) *Server {
return &Server{
hostPort: hostPort,
tracer: tracer,
Expand Down