-
-
Notifications
You must be signed in to change notification settings - Fork 85
Add OpenTelemetry span events and missing instrumentation #483
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
Summary of ChangesHello @dahlia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability of the Fedify project by integrating more comprehensive OpenTelemetry tracing. It introduces detailed span events to capture rich, structured data like full activity payloads and verification statuses, which was previously not possible with standard span attributes. Additionally, new spans have been added to cover critical operations such as document loading and cryptographic key ownership verification, providing deeper insights into the system's behavior and performance. These enhancements empower developers to build more sophisticated debugging and monitoring tools. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request significantly enhances the observability of Fedify by introducing new OpenTelemetry spans and, more importantly, span events. The added instrumentation for document loading, key ownership verification, and various ActivityPub operations will provide much richer data for debugging and monitoring. The use of span events to capture full JSON payloads is a great approach to overcome the limitations of span attributes.
The changes are well-implemented, with comprehensive test coverage using the new TestSpanExporter helper, which is a nice addition for testing. The documentation has also been thoroughly updated to reflect these new features, including a helpful example of a custom SpanExporter.
I have a couple of minor suggestions to improve the code example in the documentation for clarity and correctness. Overall, this is an excellent contribution that greatly improves the project's observability capabilities.
| interface ActivityRecord { | ||
| direction: "inbound" | "outbound"; | ||
| activity: unknown; | ||
| timestamp: Date; | ||
| verified: boolean; | ||
| } |
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.
The verified property on ActivityRecord is mandatory. This leads to hardcoding it to true for outbound activities on line 352, which can be misleading as outbound activities aren't "verified" in the same way inbound ones are. The underlying activitypub.activity.sent event doesn't include a verified attribute.
Consider making the verified property optional. This would allow you to remove the hardcoded verified: true from the outbound activity record creation, more accurately reflecting the data available from the span event.
| interface ActivityRecord { | |
| direction: "inbound" | "outbound"; | |
| activity: unknown; | |
| timestamp: Date; | |
| verified: boolean; | |
| } | |
| interface ActivityRecord { | |
| direction: "inbound" | "outbound"; | |
| activity: unknown; | |
| timestamp: Date; | |
| verified?: boolean; | |
| } |
docs/manual/opentelemetry.md
Outdated
| export class FedifyDebugExporter implements SpanExporter { | ||
| private activities: ActivityRecord[] = []; | ||
|
|
||
| export(spans: ReadableSpan[], resultCallback: (result) => void): void { |
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.
The type annotation for resultCallback is a bit loose. It can be made more specific to improve type safety and clarity for users copying this example. The result object has a code property of type ExportResultCode.
| export(spans: ReadableSpan[], resultCallback: (result) => void): void { | |
| export(spans: ReadableSpan[], resultCallback: (result: { code: ExportResultCode }) => void): void { |
Enhanced OpenTelemetry observability by adding span events to capture rich activity data and instrumenting previously uncovered operations. Span events added: - activitypub.activity.received: Records full activity JSON and verification status (activity, HTTP signatures, LD signatures) in the activitypub.inbox span - activitypub.activity.sent: Records activity JSON and target inbox in the activitypub.send_activity span - activitypub.object.fetched: Records object type and JSON-LD in the activitypub.lookup_object span New spans added: - docloader.fetch: Tracks document loading, HTTP redirects, and final document URLs - activitypub.verify_key_ownership: Tracks key ownership verification with actor ID, key ID, result, and verification method Also added comprehensive test coverage using a new TestSpanExporter helper to verify all instrumentation is properly recorded. Resolves fedify-dev#323 Co-Authored-By: Claude <[email protected]>
- Make ActivityRecord.verified optional instead of required - Add proper type annotation for resultCallback parameter - Remove hardcoded verified: true for outbound activities
…sistency This change aligns the document loader span name with the existing naming convention used by other spans in the codebase, where all ActivityPub-related operations use the 'activitypub.*' namespace.
714bf89 to
4547e9d
Compare
Summary
Enhanced OpenTelemetry observability by adding span events to capture rich activity data and instrumenting previously uncovered operations.
This PR implements the alternative approach proposed in #323, leveraging existing OpenTelemetry infrastructure instead of creating a new
FederationObserverinterface.Changes
Span events added
Span events now record complete activity JSON payloads and verification status, enabling richer observability without relying solely on span attributes (which only support primitive values):
activitypub.activity.receivedin theactivitypub.inboxspanactivitypub.activity.sentin theactivitypub.send_activityspanactivitypub.object.fetchedin theactivitypub.lookup_objectspanNew spans added
docloader.fetchfor document loader operationsactivitypub.verify_key_ownershipfor key ownership verificationTest coverage
Added comprehensive test coverage using a new
TestSpanExporterhelper inpackages/fedify/src/testing/otel.tsto verify all instrumentation is properly recorded:Documentation
Updated
docs/manual/opentelemetry.mdwith:FedifyDebugExporterimplementation showing how to build custom observability toolsUse cases
This enhancement enables developers to:
SpanExporters that process span eventsBreaking changes
None. This is a backward-compatible enhancement that adds new observability data points.
Resolves #323