Add tracing to events package, deprecate TraceID/SpanID #123
Conversation
jnschaeffer
force-pushed
the
events-tracing
branch
from
bad75f3
to
cc9afdc
Compare
This commit adds tracing using a new field, TraceContext, in ChangeMessage and EventMessage. While we could stick with TraceID and SpanID (and those fields still exist in the message and will be marshaled/unmarshaled), there are other OpenTelemetry features like the Baggage API and W3C traceparent/tracestate values that we can leverage using TraceContext instead. Moving to this allows for a tighter integration with OpenTelemetry's Go SDK as well and makes it easier for other services using the events package to add observability to their event systems. Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
jnschaeffer
force-pushed
the
events-tracing
branch
from
cc9afdc
to
d481531
Compare
| @@ -91,9 +95,13 @@ type EventMessage struct { | |||
| Source string `json:"source"` | |||
| // Timestamp is the time representing when the message was created | |||
| Timestamp time.Time `json:"timestamp"` | |||
| // TraceContext is a map of values used for OpenTelemetry context propagation. | |||
| TraceContext map[string]string `json:"traceContext"` | |||
There was a problem hiding this comment.
Is there a reason to use the underlying type of propogation.MapCarrier instead of that type directly?
There was a problem hiding this comment.
Like having TraceContext be a propagation.MapCarrier? If so, we could do that. I kind of like the idea of minimizing third-party dependencies in our message formats though.
There was a problem hiding this comment.
Yeah, my thinking is that a service using this type should not be free to just write arbitrary keys, or should need to pass through the interface the carrier provides. Looking at the otel SDK again, propogation.MapCarrier is just one implementation, so I think this is the best way?
There was a problem hiding this comment.
Thinking about it more (and also condensing an offline discussion with @adammohammed): My main argument for using builtin types like map[string]string in here is that it makes the message formats clearer for consumers that aren't using Go for writing Infratographer integrations. While it's conventional in Go to use things like time.Time for JSON data, that doesn't really tell an outside party what the shape of the data should be - to figure that out, they would need to go to the relevant documentation. Since we're not using protobuf or a similar language-agnostic method for publishing these, it's probably good to keep the barrier to understanding the message format as low as possible.
By default, otel.GetTracerProvider will return a no-op provider if one is not set. Because of this, we should create a tracer at the time the publisher is created. Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
|
Should we make this change on the new format for authorization roles as well? It has those fields. |
|
@nicolerenee I can do that; commit incoming shortly. |
Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
This PR adds tracing using a new field, TraceContext, in ChangeMessage and EventMessage. While we could stick with TraceID and SpanID (and those fields still exist in the message and will be marshaled/unmarshaled), there are other OpenTelemetry features like the Baggage API and W3C traceparent/tracestate values that we can leverage using TraceContext instead. Moving to this allows for a tighter integration with OpenTelemetry's Go SDK as well and makes it easier for other services using the events package to add observability to their event systems.