-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add OpenTelemetry instrumentation for event handlers #41
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
base: main
Are you sure you want to change the base?
Conversation
PR SummaryAdds first-class OpenTelemetry tracing integration for Commanded event processing.
Written by Cursor Bugbot for commit 3c15139. This will update automatically on new commits. Configure here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2f6c832 to
af578fb
Compare
3ca5650 to
97028f5
Compare
97028f5 to
8126b0e
Compare
8126b0e to
a62a126
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
a62a126 to
07798dd
Compare
Updated the OpenTelemetry event handler to use `attach_many` for single and batch event handlers, reducing redundancy. Adjusted tests to reflect the new handler structure, ensuring proper verification of telemetry events.
Updated the `mix.exs` file to make OpenTelemetry dependencies required instead of optional. Enhanced the documentation to reflect these changes and ensured the OpenTelemetry module integrates seamlessly with Commanded for distributed tracing.
| for handler <- :telemetry.list_handlers([]) do | ||
| :telemetry.detach(handler.id) | ||
| end | ||
| end) |
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.
Test cleanup uses wrong telemetry handler query
The on_exit callback uses :telemetry.list_handlers([]) which lists handlers for the event named [] (an empty list), not all handlers. This cleanup is ineffective. The individual test files correctly use explicit event names like [:commanded, :event, :handle, :start] in their detach_handlers/0 functions. The same pattern should be used here.
| def safe_context_propagation(metadata) when is_map(metadata) do | ||
| headers = build_headers_from_metadata(metadata) | ||
| if headers != [], do: :otel_propagator_text_map.extract(headers) | ||
| end |
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.
Context leakage in child span relationship mode
The safe_context_propagation function uses :otel_propagator_text_map.extract/1 which modifies the current process context as a side effect. When an event with a traceparent is processed, that trace context persists in the process dictionary. If a subsequent event has no traceparent, safe_context_propagation returns nil without clearing the old context, causing the new event's span to incorrectly become a child of the previous event's trace. This results in incorrect trace relationships in the tracing backend. The fix would be to use extract_to/2 like extract_span_context_for_link does, extracting into a fresh context without modifying the process dictionary.
Additional Locations (1)
…ontext handling Deleted the migration guide for `commanded-ecto-projections` as Ecto projection support is now built-in. Refactored the OpenTelemetry event handler and helper functions to streamline context propagation, ensuring clarity in parent-child span relationships. Updated tests to reflect changes in context handling.
Added new span relationship modes for OpenTelemetry setup, allowing for more flexible tracing configurations. Introduced a detach function to clean up telemetry handlers, enabling reconfiguration during tests. Updated documentation and tests to reflect these changes, ensuring clarity in usage and functionality.
| &__MODULE__.batch_telemetry_event/4, | ||
| config | ||
| ) | ||
| end |
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.
Setup ignores first telemetry attach result
The setup/1 function's spec declares it returns :ok | {:error, :already_exists}, but the implementation ignores the return value of the first :telemetry.attach_many call for single event handlers (line 29-38). Only the second attach_many result (for batch handlers) is returned. If the first attach_many fails with {:error, :already_exists} but the second succeeds, the function incorrectly returns :ok. Conversely, if the first succeeds but the second fails, the function returns an error while leaving single event handlers attached, creating inconsistent state. The function should use a with construct to properly check both results.
…ed attributes Enhanced the OpenTelemetry event handler tests by replacing hardcoded values with realistic UUIDs for event IDs, stream IDs, causation, and correlation IDs. Updated attribute assertions to reflect the new structure and naming conventions for projectors. Improved error handling tests to simulate realistic scenarios, ensuring better coverage and accuracy in telemetry data.
Signed-off-by: Yordis Prieto yordis.prieto@gmail.com