feat: add otel instrumentation to logwriter #179
Conversation
|
tried to run |
handler/subscriber/instrument.go
Outdated
| detector := lambdadetector.NewResourceDetector() | ||
| res, err := resource.New(ctx, | ||
| resource.WithDetectors(detector), | ||
| resource.WithAttributes(semconv.ServiceName("aws-sam-apps/subscriber")), |
There was a problem hiding this comment.
This should probably come from the OTEL_SERVICE_NAME environment variable or be passed in as an argument, rather than hardcoded in package.
handler/subscriber/instrument.go
Outdated
| sdktrace.WithResource(res), | ||
| ) | ||
| otel.SetTracerProvider(tracerProvider) | ||
| otelaws.AppendMiddlewares(&awsCfg.APIOptions) |
There was a problem hiding this comment.
Not sure doing this here is a good idea. InitTracing is a singleton, you would never want to instantiate it twice. It should probably be wrapped in a sync.Once. On the other hand, you may very well want different AWS clients (e.g. one per region). It's odd that you must provide an aws.Config to even initialize tracing.
handler/subscriber/instrument.go
Outdated
| return tracer, nil | ||
| } | ||
|
|
||
| func Shutdown(ctx context.Context) error { |
There was a problem hiding this comment.
You could change InitTracing from:
InitTracing(ctx context.Context, awsCfg *aws.Config) (trace.Tracer, error)
to
InitTracing(ctx context.Context) (trace.Tracer, func(context.Context) error)
The invocation would look like:
tracer, shutdownFn := InitTracing(ctx)
if tracer == nil {
err := shutdownFn()
panic(err)
}
defer shutdownFn()
By returning the shutdown callback you still have a mechanism by which to surface initialization errors, and you get rid of the tracerProvider global.
There was a problem hiding this comment.
why would I want to panic here vs. just returning an error from init? both ways the program's going to exit right?
There was a problem hiding this comment.
also the problem with the shutdown is that it needs to happen in the SIGTERM handler in the main fn instead. I could have a global variable that I assign the shutdown to in main.go but then the instrumentation code is leaking a bit.
| *Handler | ||
| } | ||
|
|
||
| func (h *InstrumentedHandler) HandleSQS(ctx context.Context, request events.SQSEvent) (response events.SQSEventResponse, err error) { |
There was a problem hiding this comment.
Uff, this one is rough. There's no instrumentation here :(
There was a problem hiding this comment.
yeah essentially all im trying to do here is replace h.HandleRequest with the instance for InstrumentedHandler vs. for the base handler unfortunately i think I need to redefine this function to make this happen? unless there's a way to re-define the exact same function but with InstrumentedHandler as the receiver. I looked online but couldn't find anything. Also I tested this without redefining it and it does fall back to the Handler's implementation of HandleRequest without instrumentation
cmd/subscriber/main.go
Outdated
| handler lambda.Handler | ||
| logger logr.Logger | ||
| entrypoint handler.Mux | ||
| shutdownFn lambda.Option |
There was a problem hiding this comment.
options []lambda.Option is less prescriptive.
cmd/subscriber/main.go
Outdated
| err := tracerShutdownFn(ctx) | ||
| return fmt.Errorf("failed to initialize tracing: %w", err) | ||
| } | ||
| shutdownFn = subscriber.FormatLambdaShutdown(ctx) |
There was a problem hiding this comment.
This feels like an excessively shallow function. You could inline it:
options = append(options, lambda.WithEnableSIGTERM(func() {
logger.V(4).Info("SIGTERM received, running shutdown")
err := shutdownFn(ctx)
if err != nil {
logger.V(4).Error(err, "tracer shutdown failed")
}
logger.V(4).Info("shutdown done running")
}))
handler/handlertest/queue.go
Outdated
| "github.com/observeinc/aws-sam-apps/handler/subscriber" | ||
| ) | ||
|
|
||
| type S3Queue struct { |
There was a problem hiding this comment.
This name is confusing. Is it S3, or is it a queue?!
handler/subscriber/instrument.go
Outdated
| propagator := propagation.NewCompositeTextMapPropagator(propagation.Baggage{}, propagation.TraceContext{}) | ||
| ctx = propagator.Extract(ctx, req.TraceContext) | ||
| } | ||
| ctx, span := h.Tracer.Start(ctx, "HandleRequest", trace.WithAttributes(attribute.String("key1", "value1"))) |
| ) | ||
|
|
||
| type SQSMessageAttributesCarrier struct { | ||
| typesMessageAttributes map[string]types.MessageAttributeValue |
There was a problem hiding this comment.
there are different types on the outbound sqs message and inbound one. had to reimplement/copy the library to handle the type difference, the lib just uses types.MessageAttributeValue but if I try to use that as the type for HandleSQSRequest then the reflex muxer matching doesn't work
handler/subscriber/queue.go
Outdated
| Client SQSClient | ||
| URL string | ||
| } | ||
|
|
||
| func (q *queueWrapper) Put(ctx context.Context, items ...any) error { | ||
| func (q *QueueWrapper) Put(ctx context.Context, items ...*Request) error { |
There was a problem hiding this comment.
Do you still need to change this from any? Looks like you no longer need to know anything about the payload.
| "github.com/aws/aws-sdk-go-v2/service/sqs" | ||
| ) | ||
|
|
||
| type SQSClient struct { |
There was a problem hiding this comment.
You don't strictly need a struct here. You can add a method to the function:
type SendMessageFunc func(context.Context, *sqs.SendMessageInput, ...func(*sqs.Options)) (*sqs.SendMessageOutput, error)
func (fn *SendMessageFunc) SendMessage(ctx context.Context, params *sqs.SendMessageInput, optFns ...func(*sqs.Options)) (*sqs.SendMessageOutput, error) {
return fn.SendMessageFunc(ctx, params, optFns...)
}
This is similar to what net/http does for HandlerFunc vs Handler.
There was a problem hiding this comment.
what's c in the above example now that there's no struct?
There was a problem hiding this comment.
derp, I swapped c for fn in the receiver, but not in the body of the method.
obs-gh-alexlew
force-pushed
the
alew/instrument-logwriter-otel-2
branch
from
5a13756
to
482b57e
Compare
|
🎉 This PR is included in version 1.7.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.