Repository navigation
feat: add OpenTelemetry instrumentation - #672
kahirokunn wants to merge 9 commits into
Conversation
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Add cloudevents/otel helpers for sending and receiving CloudEvents. Record messaging spans, metrics and exception logs, propagate trace context, and link send and process spans to event creation contexts. Signed-off-by: kahirokunn <okinakahiro@gmail.com>
There was a problem hiding this comment.
So on reviewing this, to me it seems over engineered and ultimately longterm fragile due to the coupling of messaging, rpc & http conventions.
My suggestion is as follows:
- have the encode operation add the cloud event attribute values in baggage
- have the decode operation remove the cloud event baggage items
- develop either of the below which converts baggage to attributes
- cloud events log/span processor
- baggage to attribute log/span processor
With this approach we can have the following scenarios:
Integrated transport
- application calls send
- ce lib start emit process
- ce otel extension adds cloud event info to baggage
- transport lib send invoked
- transport instrumentation creates span
- processor reads baggage & adds it as attributes
- propagator attaches info to carrier
- transport sends request
- receiver transport receives request
- propagator attaches info from carrier
- processor reads baggage & adds it as attributes as well as removes it from baggage
- handling of event completes.
Encoder
- ce encoder library called for a payload
- ce otel extension adds cloud event info to baggage
- application has an encoded payload
- application invokes transport lib send
- transport instrumentation creates span
- processor reads baggage & adds it as attributes
- propagator attaches info to carrier
- transport sends request
- receiver transport receives request
- propagator attaches info from carrier
- application creates span
- processor reads baggage & adds it as attributes as well as removes it from baggage
- ce decoder library called for a payload
- application handles decoded payload
This way the solution works for any transport & the responsibility for defining the telemetry resides with the instrumentation libraries rather than cloud events.
|
I think CloudEvents instrumentation should represent the logical event lifecycle independently of the underlying transport. OpenTelemetry's CloudEvents Semantic Conventions recommend following the Messaging Span model, including span relationships and message creation contexts. I'd prefer to follow these conventions directly, keeping CloudEvents-level observability separate from transport-level instrumentation. |
This is not documented anywhere in semantic conventions and in fact it goes the opposite direction by not documenting the lifecycle ie no span but rather what attributes can be added to spans.
The conventions are for the instrumentation which in the case of sending http request that would http, it would make no sense to have a messaging publish span for http as you don't have a messaging system.
I am following the conventions, as per https://opentelemetry.io/docs/specs/semconv/cloudevents/cloudevents-spans/#span-attributes key points bei g
Which means the attributes are added to the protocol ie if you are using rabbitmq then you have your rabbitmq span and cloud events attributes added to it.
That is exactly what i am proposing, using the extension to add to context and pulling it out when needed. Key thing is, convention is not for a span but rather attributes added to applicable spans. |
|
The Go SDK already creates CloudEvents-level PRODUCER and CONSUMER spans alongside HTTP instrumentation. I’d like to follow the same separation in the JavaScript SDK rather than limit CloudEvents instrumentation to adding attributes to transport spans. |
|
The go sdk is actually quite different to both the current otel spec & this pr. Go is just creating a span with the cloud event attributes and nothing else so no connection to messaging etc. Note I can see the benefit to this approach on client side as it avoids the need to attach attributes after the payload has been encoded but on receiver side it fells un-necessary as what do you gain? Also on looking at the otel spec history it appears that Go is following a much older spec which was removed in open-telemetry/semantic-conventions#1182. In fact it was written by the same author of the go sdk, just 3 years later. |
|
Thanks for pointing out the spec history. On the receiver side, the value I’m aiming for is batch-aware processing: one processing span linked to each event’s available creation context, with batch size and per-event attributes. This follows a similar opt-in, library-level instrumentation pattern to Prisma for ORM operations and BullMQ for job processing. I’d like to provide this as reusable, opt-in instrumentation so applications don’t have to implement that logic themselves. Where existing instrumentation already covers this, the wrapper can simply be omitted. |
Should be already covered out of the box as per messaging spec https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#batch-receiving in particular https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#recording-per-message-attributes-on-batch-operations which describes how to capture the message level details. This can all be done without instrumentation & instead achieved at the propagation layer via a custom propagator.
The issue is not about providing the functionality but rather what is the responsibility of cloudevents to provide.
Existing instrumentation will only cover the standard messaging use case ie creating spans however a cloudevents propagator is needed to create the links based on the payload & processor to add attributes to the messaging send span. As such i feel that the extension should just be:
|
Proposed Changes
Add optional OpenTelemetry wrappers for sending and processing CloudEvents through
cloudevents/otel.Description
The wrappers record spans, messaging metrics and exception logs. Sends use the active context as their parent and link to existing event creation contexts without replacing them.
Depends on #657. Related to #355.