server icon indicating copy to clipboard operation
server copied to clipboard

[WIP] OTEL Observability

Open IshanDaga opened this issue 1 year ago • 4 comments

Feature

Using OTEL lib to instrument the server. Happy to take suggestions on things that we should track, metrics to add, and how to allow maximum configurability for the Tracer.

Tasks

  • [x] Add context object to each packet
  • [x] Add OTEL config options to server.Options struct
  • [ ] Add spans for client events
  • [ ] Add metrics tracking using OTEL Meters API
  • [ ] Instrument slog handler

Pending decisions

  • [ ] should we add another external dependency like https://github.com/ttys3/slogx to internally instrument slog, or should we leave that to the user to implement within their own hook implementations ?

IshanDaga avatar Sep 09 '23 17:09 IshanDaga

@dgduncan @mochi-co Another approach to this could be to have a StartTrace hook that is called after receivePacket We could also add in a trace-modifier hook of sorts, that allows someone to insert the TraceID into the header for MQTTv5 or modify the packet to include TraceID bytes with a separator like in #227

Looking for initial comments and thoughts :)

IshanDaga avatar Sep 09 '23 18:09 IshanDaga

@mochi-co hey I'm having trouble with the test cases involving sample packets. Most are failing on the require.Equal since now a packet from the server has a context, but the sample packet doesn't.

There's quite a few of these cases, any suggestions on how I could move past this without breaking the current testing method ?

IshanDaga avatar Oct 21 '23 13:10 IshanDaga

@IshanDaga SImplest solution would be to only populate context if OTEL tracing is enabled:

Index: clients.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/clients.go b/clients.go
--- a/clients.go	(revision 9d6c78915853d616b25757776b6945885b48df22)
+++ b/clients.go	(date 1697972510469)
@@ -454,7 +454,9 @@
 func (cl *Client) ReadPacket(fh *packets.FixedHeader) (pk packets.Packet, err error) {
 	atomic.AddInt64(&cl.ops.info.PacketsReceived, 1)
 
-	pk.Ctx, pk.Cancel = context.WithCancel(cl.State.open)
+	if cl.ops.options.OTELTracing {
+		pk.Ctx, pk.Cancel = context.WithCancel(cl.State.open)
+	}
 
 	pk.ProtocolVersion = cl.Properties.ProtocolVersion // inherit client protocol version for decoding
 	pk.FixedHeader = *fh

mochi-co avatar Oct 22 '23 11:10 mochi-co

@mochi-co makes sense, thanks !

IshanDaga avatar Oct 22 '23 18:10 IshanDaga

@IshanDaga This PR is a bit out of date, I recommend closing it and opening a new one when there's more changes 👍🏻

mochi-co avatar May 12 '24 09:05 mochi-co

@mochi-co makes sense, closing for now, will get back on this as soon as i have a little more free time

IshanDaga avatar May 12 '24 09:05 IshanDaga