Added OTLP Attributes and testing #10
Conversation
* Changed mix dependency for Commanded to get unreleased merge DavyDog made for event store * Changed callback module to capture correct set of events * Modified attributes to match messaging semantics like others w/i this library
lib/commanded/event_handler.ex
Outdated
| @@ -34,22 +34,28 @@ defmodule OpentelemetryCommanded.EventHandler do | |||
|
|
|||
| def handle_start(_event, _measurements, meta, _) do | |||
| event = meta.recorded_event | |||
|
|
|||
| # TODO: do we want to trap this with helpful message about needing Middleware? | |||
There was a problem hiding this comment.
@davydog187 and/or @bryannaegele
If the user hasn't added the middleware to their app this will cause an exception and kills this process.
Should I trap here?
There was a problem hiding this comment.
My understanding of the design philosophy behind Otel is to fail gracefully, i.e. it shouldn't impact the uptime of the system. So catching/rescuing any issues here and downgrading the system with a helpful error or warning log seems like a good move
lib/commanded/event_store.ex
Outdated
| # TODO does this need to change based on the event? Or maybe `internal`? | ||
| "messaging.operation": "receive", |
There was a problem hiding this comment.
@bryannaegele
For spans created by a messaging system all have operation types from that pool (send, receive, subscribe)?
This doesn't seem to fit well. It seems an "internal" span w/i the CQRS messaging system.
Thoughts?
There was a problem hiding this comment.
I don't know where subscribe comes from. Messaging operations are send, receive, process. Span type of internal is only appropriate where a function is calling another function. If there's a message or operation crossing a system boundary, e.g. to the database, then it's not internal.
CQRS is a bit of a hybrid between Messaging and RPC, so I'm not sure of all the specifics of how it should be treated. You can always solicit opinions in the #opentelemetry channel of the CNCF slack.
lib/commanded/event_store.ex
Outdated
| "messaging.operation": "receive", | ||
| "messaging.message_id": event.causation_id, | ||
| "messaging.conversation_id": event.correlation_id, | ||
| # "messaging.destination": meta.handler_module, |
There was a problem hiding this comment.
@davydog187 and/or @bryannaegele
The above question about the span kind being "internal" is supported by the fact there is no known source or destination for this span.
| # TODO add back | ||
| # consistency: meta.consistency, | ||
| # TODO add this back into commanded | ||
| # "event.last_seen": meta.last_seen_event |
There was a problem hiding this comment.
Left @davydog187 's comments.
There was a problem hiding this comment.
@bryannaegele
After reading a ton, I think that I should remove the messaging. prefix from the Commanded attributes.
This would also allow easier query building too I would think
Thoughts?
e.g.
"messaging.commanded.application": meta.application,
"messaging.commanded.event": recorded_event.event_type,
"messaging.commanded.event_id": recorded_event.event_id,
"messaging.commanded.event_number": recorded_event.event_number,
"messaging.commanded.process_uuid": meta.process_uuid,
"messaging.commanded.stream_id": recorded_event.stream_id,
"messaging.commanded.stream_version": recorded_event.stream_version,
"messaging.commanded.handler_name": meta.process_manager_name
should be
"commanded.application": meta.application,
"commanded.event": recorded_event.event_type,
"commanded.event_id": recorded_event.event_id,
"commanded.event_number": recorded_event.event_number,
"commanded.process_uuid": meta.process_uuid,
"commanded.stream_id": recorded_event.stream_id,
"commanded.stream_version": recorded_event.stream_version,
"commanded.handler_name": meta.process_manager_name
| application: meta.application, | ||
| "causation.id": context.causation_id, | ||
| "correlation.id": context.correlation_id, | ||
| "aggregate.function": context.function, |
There was a problem hiding this comment.
do we still want these two fields?
There was a problem hiding this comment.
aggregate.function": context.function is still there, just massaged to fit naming for messaging attributes:
"messaging.commanded.function": context.functionI can restore context.lifespan but while testing, didn't see it's value as an attribute since it doesn't effect the processing of the current span. Thoughts?
… boundaries. Fixed and added tests. Safely handle case where the user hasn't added middleware `middleware(OpentelemetryCommanded.Middleware)` to the router
lib/commanded/event_store.ex
Outdated
|
|
||
| attributes = [ | ||
| "messaging.system": "commanded", | ||
| "messaging.protocol": "cqrs", |
There was a problem hiding this comment.
cqrs isn't a transport protocol, it's a pattern. This attribute should probably not be included.
lib/commanded/event_store.ex
Outdated
| # TODO does this need to change based on the event? Or maybe `internal`? | ||
| "messaging.operation": "receive", |
There was a problem hiding this comment.
I don't know where subscribe comes from. Messaging operations are send, receive, process. Span type of internal is only appropriate where a function is calling another function. If there's a message or operation crossing a system boundary, e.g. to the database, then it's not internal.
CQRS is a bit of a hybrid between Messaging and RPC, so I'm not sure of all the specifics of how it should be treated. You can always solicit opinions in the #opentelemetry channel of the CNCF slack.
Since CQRS is considered a messaging system, changed emitted attributes to follow that semantic and added tests.
Assuming the testing of tracing events is adequate, all that's needed is to assert that we've attached correctly to the telemetry callbacks and that the spans are correctly constructed in the three cases: success, error, exception
closes: #1 , #2 , #3 , #4