support stream event log #1488
support stream event log #1488
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces detailed event tracing for gRPC streaming in the SOFA-RPC framework. It adds new streaming event constants, enhances interceptors to record granular send/receive events, and introduces new JSON encoders and log types for event reporting. Application configuration and integration tests are updated to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant ClientInterceptor
participant RpcInternalContext
participant SofaTracerSpan
participant Reporter
ClientInterceptor->>RpcInternalContext: Capture context at call start
ClientInterceptor->>SofaTracerSpan: Create span for send/receive event
ClientInterceptor->>Reporter: Report event (send/receive) with event data
Note over ClientInterceptor,Reporter: On error, mark event as failed and report
sequenceDiagram
participant ServerInterceptor
participant RpcInternalContext
participant SofaTracerSpan
participant Reporter
ServerInterceptor->>RpcInternalContext: Set context as provider side
ServerInterceptor->>SofaTracerSpan: Create span for send/receive event
ServerInterceptor->>Reporter: Report server event with event data
Note over ServerInterceptor,Reporter: Context is propagated and cleaned up per event
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java (1)
140-155: 🛠️ Refactor suggestionUnbalanced push/clear leads to empty trace stack
Inside
onMessageyou do:
sofaTraceContext.push(clientSpan);...sofaTraceContext.clear();
clear()empties the whole stack, so subsequent messages (and finallyonClose) no longer have the server span as parent; this breaks nested tracing for long streams.Replace
clear()with a symmetricalpop():- sofaTraceContext.clear(); + sofaTraceContext.pop();This preserves the parent span and keeps the stack invariant.
♻️ Duplicate comments (1)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java (1)
218-252: Duplicate issue: null-check fororiginalSpanin receive pathSame NPE risk as send path – wrap
originalSpan.addEvent(spanEventData)with a
null guard.
🧹 Nitpick comments (14)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/RpcClientEventJsonEncoder.java (2)
1-39: Consider adding null check for the span parameter.The encoder implementation is clean and follows the template method pattern effectively. However, there's no validation to ensure the
spanparameter isn't null before accessing its methods.@Override public String encode(SofaTracerSpan span) throws IOException { + if (span == null || span.getEventData() == null) { + return "{}"; + } buffer.reset(); buffer.appendBegin(RpcSpanTags.TIMESTAMP, span.getEventData().getTimestamp()); appendSlot(span); buffer.appendEnd(); return buffer.toString(); }
28-38: Consider refactoring identical encode implementations in client and server encoders.The
encodeimplementation is identical between this class andRpcServerEventJsonEncoder. If this is the only difference between client and server encoders, consider creating a factory method or using a strategy pattern to avoid duplicate code.tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/RpcServerEventJsonEncoder.java (2)
1-39: Consider adding null check for the span parameter.The encoder implementation is clean and follows the template method pattern effectively. However, there's no validation to ensure the
spanparameter isn't null before accessing its methods.@Override public String encode(SofaTracerSpan span) throws IOException { + if (span == null || span.getEventData() == null) { + return "{}"; + } buffer.reset(); buffer.appendBegin(RpcSpanTags.TIMESTAMP, span.getEventData().getTimestamp()); appendSlot(span); buffer.appendEnd(); return buffer.toString(); }
28-38: Consider refactoring identical encode implementations in client and server encoders.The
encodeimplementation is identical between this class andRpcClientEventJsonEncoder. If this is the only difference between client and server encoders, consider refactoring to a common implementation to avoid code duplication.test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleGenericStreamTest.java (1)
79-80: Consider parameterizing the timeout value.The timeout value (1000000) is quite large and appears as a magic number in the code. Consider extracting this to a named constant to improve readability and maintenance.
+ private static final int STREAM_TEST_TIMEOUT = 1000000; ... - .setTimeout(1000000) + .setTimeout(STREAM_TEST_TIMEOUT)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/factory/ReporterFactory.java (2)
34-41: Static-finalREPORT_TYPEprevents live config refresh
REPORT_TYPEis captured once at class-loading time. If your platform supports
dynamic refresh ofsofa.rpc.tracer.exposeType, changes will no longer take
effect at runtime.Consider keeping it non-final or resolving the config inside
build(...)so
hot-reload remains possible.
53-57: Typos & memory reporter gap inbuildEventReport
- Parameter name
evenLogTypeis a typo – should beeventLogType.- The method unconditionally returns a
SpanEventDiskReporter; if
REPORT_TYPE == "MEMORY"a memory reporter variant will still be
desirable for symmetry withbuild().-public static Reporter buildEventReport(String evenLogType, ... - return new SpanEventDiskReporter(evenLogType, ..., evenLogType); +public static Reporter buildEventReport(String eventLogType, ... + return new SpanEventDiskReporter(eventLogType, ..., eventLogType);Optionally, branch on
REPORT_TYPEto construct an in-memory reporter when
requested.core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java (1)
140-155: Consider documenting the new event-type constantsThe four streaming event constants look correct and follow the existing naming convention, but unlike the surrounding
INVOKER_TYPE_*constants they lack any Javadoc that explains when each one is emitted (send vs. receive, client vs. server). A short comment block (or reuse of an existing one) will help future contributors use the right constant and avoid confusion with similarly named tags.tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/RpcSofaTracer.java (4)
100-109: Reporter chain now includes event reporters – double-check log volumeAdding separate reporters for every stream message can explode disk usage in busy systems.
Consider:
- Making the event reporter’s rolling policy stricter (e.g.,
size-basedor hourly).- Adding a switch in
sofa-tracer.propertiesto disable event logs in production when not needed.
120-127: Provide explicit visibility for new encoder helpers
getClientEventEncoder()andgetServerEventEncoder()areprotected, consistent with existing helpers.
If subclasses are not expected to override them, mark themfinalto prevent accidental changes.- protected SpanEncoder<SofaTracerSpan> getClientEventEncoder() { + protected final SpanEncoder<SofaTracerSpan> getClientEventEncoder() {…and similarly for
getServerEventEncoder.
160-168: Typo in local variable name
evenLogType→eventLogType- String evenLogType = eventRpcTracerLogEnum.getDefaultLogName(); + String eventLogType = eventRpcTracerLogEnum.getDefaultLogName(); - return ReporterFactory.buildEventReport(evenLogType, digestRollingPolicy, digestLogReserveConfig, spanEncoder); + return ReporterFactory.buildEventReport(eventLogType, digestRollingPolicy, digestLogReserveConfig, spanEncoder);
417-433: R0 phase calculation: guard against negative elapsed timeIf
System.nanoTime()overflows (very long-lived JVM) or clock is adjusted, the subtraction could be negative.
ConsiderMath.max(0, streamFirstRespTime)before converting.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java (2)
197-214: MissingRpcInternalContext.setContextfor sender thread
sendMessageupdatesinternalContextbut does not callRpcInternalContext.setContext(internalContext).
If the caller code later accessesRpcInternalContext, it will operate on a different (empty) thread-local instance.Add:
RpcInternalContext.setContext(internalContext); try { super.sendMessage(message); } finally { RpcInternalContext.removeContext(); }
221-228: Null check forclientSpanbeforeaddEventSame NPE risk as noted earlier – guard the call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
all/pom.xml(1 hunks)bom/pom.xml(1 hunks)core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java(2 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java(7 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java(8 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java(1 hunks)test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleGenericStreamTest.java(2 hunks)test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleStubStreamTest.java(3 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/RpcSofaTracer.java(5 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/factory/ReporterFactory.java(3 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/AbstractRpcEventJsonEncoder.java(1 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/RpcClientEventJsonEncoder.java(1 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/RpcServerEventJsonEncoder.java(1 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/RpcEventTags.java(1 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/TracerRecord.java(2 hunks)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/type/RpcTracerLogEnum.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/factory/ReporterFactory.java (1)
core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1)
RpcConfigKeys(28-155)
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleStubStreamTest.java (1)
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java (1)
RpcConstants(28-747)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/RpcClientEventJsonEncoder.java (1)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/RpcSpanTags.java (1)
RpcSpanTags(24-192)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/RpcServerEventJsonEncoder.java (1)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/RpcSpanTags.java (1)
RpcSpanTags(24-192)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java (1)
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java (1)
RpcConstants(28-747)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java (6)
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java (1)
RpcConstants(28-747)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/RpcEventTags.java (1)
RpcEventTags(23-39)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/TripleExceptionUtils.java (1)
TripleExceptionUtils(26-48)core/api/src/main/java/com/alipay/sofa/rpc/context/RpcInternalContext.java (1)
RpcInternalContext(40-503)core/api/src/main/java/com/alipay/sofa/rpc/context/RpcInvokeContext.java (1)
RpcInvokeContext(32-471)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleContants.java (1)
TripleContants(22-30)
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/AbstractRpcEventJsonEncoder.java (2)
core/common/src/main/java/com/alipay/sofa/rpc/common/utils/CommonUtils.java (1)
CommonUtils(30-278)tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/RpcSpanTags.java (1)
RpcSpanTags(24-192)
🔇 Additional comments (13)
all/pom.xml (1)
65-65: Version update aligns with tracer enhancementsThe version update of
tracer.versionfrom 3.1.6 to 3.1.10 aligns with the version update in the BOM file and supports the new streaming event tracing capabilities being introduced in this PR.bom/pom.xml (1)
58-58: Consistent version update in BOMThe tracer version update in the BOM file ensures consistency with the same update in all/pom.xml, maintaining proper dependency management across the project.
tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/TracerRecord.java (2)
21-21: Added documentation for new stream response timingGood documentation for the new
R0value that describes stream first response latency between client and server.
37-37: Added new timing constant for stream first responseThe addition of
R0at the beginning of the enum values introduces timing measurement specific to streaming response latency. This aligns with the PR objective to support stream event logging.tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/type/RpcTracerLogEnum.java (1)
31-32: Added new log types for client and server eventsThe addition of
RPC_CLIENT_EVENTandRPC_SERVER_EVENTenum constants supports the new event reporting capability for streaming RPC calls. These constants properly follow the established pattern with appropriate log names and rolling policies.test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleStubStreamTest.java (4)
87-87: Added explicit application configuration for consumerCreating an explicit
ApplicationConfigwith a specific app name enhances traceability in the events and logs.
90-92: Enhanced consumer configuration with application and URL parametersSetting the application configuration and adding the application name as a URL parameter improves context propagation for tracing.
108-110: Consistent configuration for second consumerThe same application configuration is applied to the second consumer with appropriate application name parameters, ensuring consistent behavior.
125-130: Added unary RPC test caseThe new test method
testTripleSayHello()covers the basic unary RPC path, which complements the existing stream test cases. This provides better test coverage for the entire RPC flow.tracer/tracer-opentracing/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/tags/RpcEventTags.java (1)
1-39: Well-structured and clear tag definitions for streaming RPC event logging.The new
RpcEventTagsclass provides a well-organized set of constants that will standardize the tagging of RPC event logs. These tags are essential for implementing detailed event tracing across streaming RPC calls, providing consistent metadata fields for stream identification, event sequencing, and status tracking.test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleGenericStreamTest.java (2)
65-72: Good addition of explicit application configuration for the server.Adding an explicit application name for the server component improves the test by clearly identifying the service provider in traces and logs, which is essential for proper event tracking in streaming calls.
74-81: Enhance test configuration with proper application naming and timeout.The changes properly configure the consumer with:
- A distinct application name ("triple-client")
- A longer timeout (1000000) suitable for streaming tests
- An explicit appName parameter in the direct URL
These improvements ensure proper identification of client/server components in event logs and provide sufficient time for streaming operations to complete.
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java (1)
695-697: 👍 Internal-key constant aligns with prefix conventionThe new
INTERNAL_KEY_CLIENT_FIRST_STREAM_RESP_NANOrespects the_prefix rule enforced byRpcInternalContextand fills a gap in the phase-time metrics. No issues spotted.
...remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java
Show resolved
Hide resolved
...c/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/AbstractRpcEventJsonEncoder.java
Show resolved
Hide resolved
...c/main/java/com/alipay/sofa/rpc/tracer/sofatracer/log/event/AbstractRpcEventJsonEncoder.java
Outdated
Show resolved
Hide resolved
...emoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java
Show resolved
Hide resolved
...emoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java
Outdated
Show resolved
Hide resolved
...ting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java
Show resolved
Hide resolved
...ting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java
Show resolved
Hide resolved
Support stream rpc event log and enhance tracer.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores