-
Notifications
You must be signed in to change notification settings - Fork 19
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.
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 tracing to events package, deprecate TraceID/SpanID #123
Add tracing to events package, deprecate TraceID/SpanID #123
Conversation
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 <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use the underlying type of propogation.MapCarrier
instead of that type directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Signed-off-by: John Schaeffer <[email protected]>
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 <[email protected]>
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.