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:
|
|
My concern is using Baggage solely to pass CloudEvents metadata to a local processor. With baggage propagation enabled, that metadata could also be sent to downstream services. Opting into instrumentation shouldn’t implicitly opt applications into propagating event metadata downstream. I’d prefer to attach the batch attributes and links directly when the opt-in wrapper creates the processing span. This keeps the integration simple, avoids introducing Baggage and an additional processor, and doesn’t depend on other instrumentation providing an appropriate processing span. |
Then have the processor remove before it sending. Alternatively we can add it to the context instead that way it is not propagated. I do like baggage so that reciever spans can have access to the properties without parsing the messages.
I don't follow how this differs to what i said, under my proposal when processing a message the propagator extracts the context from the message, once extracted links can be created with the desired attributes.
You are right it doesn't rely on other instrumentation creating the span but i see that as a major positive as that way cloud events doesn't need to duplicate implementation & avoids chance of spans being double instrumented. I don't see what is complex from an app developer side, the steps would be:
Nothing else needs to be done so it is almost as simple (1 additional line) as directly creating the sending span but significantly decreases the code base needing to be maintained by cloudevents. |
|
Thanks for clarifying. I’d like to support enriching existing spans while keeping The wrapper’s purpose is to capture the event or batch handler’s own duration and failures, with links to each event’s available creation context. That processing can be a distinct operation from the enclosing HTTP request. |
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.