support span event #557
support span event #557
Conversation
WalkthroughThis change introduces support for event reporting in the tracing system. The Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Tracer as SofaTracer
participant Span as SofaTracerSpan
participant EventReporter as SpanEventDiskReporter
App->>Tracer: Create SofaTracer (with event reporters)
App->>Span: Create SofaTracerSpan
App->>Span: addEvent(eventData)
Span->>Tracer: reportEvent(span)
Tracer->>EventReporter: digestReport(span)
EventReporter->>EventReporter: (async) Write event data to disk
Suggested labels
Poem
✨ Finishing Touches
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:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SpanEventData.java (2)
42-80: Simple getters and setters look good, but consider immutability option.The getter and setter methods are correctly implemented. However, the timestamp should ideally be set at construction time and made immutable since it represents when the event occurred.
Consider using a constructor to set the timestamp instead of a setter:
-public void setTimestamp(long timestamp) { - this.timestamp = timestamp; -} +/** + * Creates a new span event data with current timestamp. + */ +public SpanEventData() { + this.timestamp = System.currentTimeMillis(); +} + +/** + * Creates a new span event data with specified timestamp. + * + * @param timestamp the timestamp for this event + */ +public SpanEventData(long timestamp) { + this.timestamp = timestamp; +}
34-35: Consider using a more precise timestamp format.The timestamp is stored as a long, presumably milliseconds since epoch. For high-performance tracing systems, microsecond or nanosecond precision might be beneficial.
Consider using the
Timestampclass that's already imported but not used:-private long timestamp; +private Timestamp timestamp; -public long getTimestamp() { - return timestamp; -} +public long getTimestamp() { + return timestamp.getTime(); +}tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java (2)
35-38: Implementation looks good, but could use a more descriptive class Javadoc.The implementation of the
ClientSpanEventEncoderclass has a clear purpose, but the class Javadoc could be expanded to better explain its role in event processing./** * The type Client span event encoder. * - * @author yuqian - * @version : ClientSpanEventEncoder.java, v 0.1 2025-03-10 17:02 yuqian Exp $$ + * This encoder serializes SofaTracerSpan event data into a formatted string for disk-based reporting. + * It handles encoding of span event timestamps, trace/span IDs, and various event tag types + * (string, boolean, numeric) along with business baggage information. + * + * @author yuqian + * @version : ClientSpanEventEncoder.java, v 0.1 2025-03-10 17:02 yuqian Exp $$ */
40-73: The encode method implementation is solid but has minor optimization opportunities.The implementation properly encodes all necessary span event data, but there are a few areas for improvement:
- The code creates separate StringBuilder instances for boolean and number tags, even though a similar pattern is used. Consider extracting a helper method to reduce duplication:
@Override public String encode(SofaTracerSpan span) throws IOException { SofaTracerSpanContext spanContext = span.getSofaTracerSpanContext(); xsb.reset(); // xsb.append(Timestamp.format(span.getEventData().getTimestamp())); //traceId xsb.append(spanContext.getTraceId()); //spanId xsb.append(spanContext.getSpanId()); //tags string xsb.append(StringUtils.mapToString(span.getEventData().getEventTagWithStr())); - //tags bool - Map<String, Boolean> tagsBool = span.getEventData().getEventTagWithBool(); - StringBuilder tagsBoolBuild = new StringBuilder(); - for (Map.Entry<String, Boolean> entry : tagsBool.entrySet()) { - tagsBoolBuild.append(entry.getKey()).append(StringUtils.EQUAL) - .append(entry.getValue().toString()).append(StringUtils.AND); - } - xsb.append(tagsBoolBuild.toString()); - - //tags number - Map<String, Number> tagsNum = span.getEventData().getEventTagWithNumber(); - StringBuilder tagsNumBuild = new StringBuilder(); - for (Map.Entry<String, Number> entry : tagsNum.entrySet()) { - tagsNumBuild.append(entry.getKey()).append(StringUtils.EQUAL) - .append(entry.getValue().toString()).append(StringUtils.AND); - } - xsb.append(tagsNumBuild.toString()); + //tags bool + xsb.append(encodeMapToString(span.getEventData().getEventTagWithBool())); + //tags number + xsb.append(encodeMapToString(span.getEventData().getEventTagWithNumber())); //baggage Map<String, String> baggage = spanContext.getBizBaggage(); xsb.appendEnd(StringUtils.mapToString(baggage)); return xsb.toString(); } +/** + * Helper method to encode a map to a string representation + */ +private <T> String encodeMapToString(Map<String, T> map) { + StringBuilder sb = new StringBuilder(); + for (Map.Entry<String, T> entry : map.entrySet()) { + sb.append(entry.getKey()).append(StringUtils.EQUAL) + .append(entry.getValue().toString()).append(StringUtils.AND); + } + return sb.toString(); +}
- The
xsbfield could be made final since it's not reassigned.tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (2)
184-184: Commented-out reportEvent code should be removed.There's a commented-out call to a non-existent method
reportEvent()in the finish method.public void finish(long endTime) { this.setEndTime(endTime); //Key record:report span - // reportEvent(); this.sofaTracer.reportSpan(this); SpanExtensionFactory.logStoppedSpan(this); }
194-210: The addEvent method implementation looks good but could use enhanced validation.The method appropriately clones the span and reports the event, with basic validation. However, it could benefit from additional validation of the event data structure.
public void addEvent(SpanEventData eventData) { if (eventData == null) { SelfLog.error("event is null"); return; } + + // Validate event timestamp + if (eventData.getTimestamp() <= 0) { + SelfLog.error("event timestamp must be positive"); + return; + } if (this.endTime > 0 && System.currentTimeMillis() > this.endTime) { // span is closed, can not report event SelfLog.error("span is closed, can not add event"); return; } SofaTracerSpan clonedSpan = this.cloneInstance(); clonedSpan.setEventData(eventData); this.sofaTracer.reportEvent(clonedSpan); }tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (1)
114-123: statisticReport implementation is minimal.The statisticReport method is implemented as a no-op (just returns), which is appropriate if event reporting doesn't require statistics. However, consider adding a comment explaining why this is a no-op.
@Override public void statisticReport(SofaTracerSpan span) { + // Events don't require statistical reporting, so this is intentionally a no-op return; }tracer-core/src/main/java/com/alipay/common/tracer/core/SofaTracer.java (2)
42-42: Import statement consolidation.The various java.util.* imports have been consolidated to a wildcard import. While this simplifies the import section, it may make it less clear which specific classes from java.util are being used.
Consider keeping explicit imports for clarity, especially for core classes like List, Map, etc.
174-197: Potential code duplication between reportSpan and reportEvent.The reportEvent method is nearly identical to reportSpan, which creates maintenance challenges. Consider refactoring to reduce duplication.
Extract common logic into a private method:
+private void reportToReporter(SofaTracerSpan span, Reporter clientReporter, Reporter serverReporter) { + if (span == null) { + return; + } + // sampler is support & current span is root span + if (sampler != null && (span.isClient() && span.getParentSofaTracerSpan() == null)) { + span.getSofaTracerSpanContext().setSampled(sampler.sample(span).isSampled()); + } + //invoke listener + this.invokeReportListeners(span); + if (span.isClient() + || this.getTracerType().equalsIgnoreCase(ComponentNameConstants.FLEXIBLE)) { + if (clientReporter != null) { + clientReporter.report(span); + } + } else if (span.isServer()) { + if (serverReporter != null) { + serverReporter.report(span); + } + } else { + //ignore, do not statical + SelfLog.warn("Span reported neither client nor server.Ignore!"); + } +} public void reportSpan(SofaTracerSpan span) { - if (span == null) { - return; - } - // //sampler is support & current span is root span - if (sampler != null && (span.isClient() && span.getParentSofaTracerSpan() == null)) { - span.getSofaTracerSpanContext().setSampled(sampler.sample(span).isSampled()); - } - //invoke listener - this.invokeReportListeners(span); - if (span.isClient() - || this.getTracerType().equalsIgnoreCase(ComponentNameConstants.FLEXIBLE)) { - if (this.clientReporter != null) { - this.clientReporter.report(span); - } - } else if (span.isServer()) { - if (this.serverReporter != null) { - this.serverReporter.report(span); - } - } else { - //ignore ,do not statical - SelfLog.warn("Span reported neither client nor server.Ignore!"); - } + reportToReporter(span, this.clientReporter, this.serverReporter); } public void reportEvent(SofaTracerSpan span) { - if (span == null) { - return; - } - // //sampler is support & current span is root span - if (sampler != null && (span.isClient() && span.getParentSofaTracerSpan() == null)) { - span.getSofaTracerSpanContext().setSampled(sampler.sample(span).isSampled()); - } - //invoke listener - this.invokeReportListeners(span); - if (span.isClient() - || this.getTracerType().equalsIgnoreCase(ComponentNameConstants.FLEXIBLE)) { - if (this.clientEventReporter != null) { - this.clientEventReporter.report(span); - } - } else if (span.isServer()) { - if (this.serverEventReporter != null) { - this.serverEventReporter.report(span); - } - } else { - //ignore ,do not statical - SelfLog.warn("Span reported neither client nor server.Ignore!"); - } + reportToReporter(span, this.clientEventReporter, this.serverEventReporter); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-datasource-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-dubbo-2.6.x-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-dubbo-common-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-dubbo-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-flexible-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-httpclient-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-kafkamq-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-mongodb-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-okhttp-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-rabbitmq-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-redis-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-resttmplate-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-rocketmq-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-spring-cloud-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-springmessage-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-springmvc-plugin/pom.xml(1 hunks)sofa-tracer-plugins/sofa-tracer-zipkin-plugin/pom.xml(1 hunks)tracer-all/pom.xml(1 hunks)tracer-core/pom.xml(1 hunks)tracer-core/src/main/java/com/alipay/common/tracer/core/SofaTracer.java(10 hunks)tracer-core/src/main/java/com/alipay/common/tracer/core/constants/SofaTracerConstant.java(1 hunks)tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java(1 hunks)tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java(10 hunks)tracer-core/src/main/java/com/alipay/common/tracer/core/span/SpanEventData.java(1 hunks)tracer-core/src/test/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporterTest.java(1 hunks)tracer-core/src/test/java/com/alipay/common/tracer/core/span/SofaTracerSpanTest.java(5 hunks)tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/SofaTracerTest.java(6 hunks)tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java(1 hunks)tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/type/TracerTestLogEnum.java(1 hunks)tracer-extensions/pom.xml(1 hunks)tracer-sofa-boot-starter/pom.xml(1 hunks)tracer-test/core-test/pom.xml(1 hunks)tracer-test/log4j-test/pom.xml(1 hunks)tracer-test/log4j2-test/pom.xml(1 hunks)tracer-test/logback-test/pom.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tracer-core/src/main/java/com/alipay/common/tracer/core/SofaTracer.java (2)
tracer-core/src/main/java/com/alipay/common/tracer/core/constants/ComponentNameConstants.java (1)
ComponentNameConstants(24-64)tracer-core/src/main/java/com/alipay/common/tracer/core/appender/self/SelfLog.java (1)
SelfLog(30-153)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (61)
tracer-test/core-test/pom.xml (1)
7-7: Validated parent version bump to 3.1.10
The update correctly aligns this test module with the new release version across the project.tracer-test/logback-test/pom.xml (1)
7-7: Validated parent version bump to 3.1.10
This change keeps the logback-test module in sync with the coordinated version update.tracer-test/log4j-test/pom.xml (1)
7-7: Validated parent version bump to 3.1.10
The version bump is correct and maintains consistency for the log4j-test module.sofa-tracer-plugins/sofa-tracer-datasource-plugin/pom.xml (1)
8-8: Validated parent version bump to 3.1.10
The datasource plugin now correctly references the updated parent version.sofa-tracer-plugins/sofa-tracer-zipkin-plugin/pom.xml (1)
8-8: Validated parent version bump to 3.1.10
The zipkin plugin’s POM correctly reflects the new parent version.sofa-tracer-plugins/sofa-tracer-okhttp-plugin/pom.xml (1)
8-8: Align parent POM version to 3.1.10
This bump ensures the okhttp plugin module is in sync with the coordinated release across all SOFA tracer artifacts.tracer-sofa-boot-starter/pom.xml (1)
8-8: Align parent POM version to 3.1.10
Consistent versioning across modules avoids cross-version mismatches in the boot-starter.sofa-tracer-plugins/sofa-tracer-redis-plugin/pom.xml (1)
8-8: Align parent POM version to 3.1.10
Version bump matches the overall project upgrade.sofa-tracer-plugins/sofa-tracer-httpclient-plugin/pom.xml (1)
8-8: Align parent POM version to 3.1.10
Ensures HTTP client plugin is compatible with the new core tracer release.sofa-tracer-plugins/sofa-tracer-mongodb-plugin/pom.xml (1)
8-8: Align parent POM version to 3.1.10
Keeps the MongoDB plugin in lockstep with the updated project version.sofa-tracer-plugins/sofa-tracer-springmessage-plugin/pom.xml (1)
8-8: Consistent parent version bump
The parent<version>has been updated from3.1.9to3.1.10, aligning this module with the rest of the release.sofa-tracer-plugins/sofa-tracer-spring-cloud-plugin/pom.xml (1)
8-8: Consistent parent version bump
The parent<version>has been updated from3.1.9to3.1.10, matching the coordinated release across all modules.sofa-tracer-plugins/sofa-tracer-dubbo-common-plugin/pom.xml (1)
8-8: Consistent parent version bump
The parent<version>has been updated from3.1.9to3.1.10, in line with the global version update.sofa-tracer-plugins/sofa-tracer-rocketmq-plugin/pom.xml (1)
8-8: Consistent parent version bump
The parent<version>has been updated from3.1.9to3.1.10, keeping this plugin in sync with the project release.pom.xml (1)
12-12:✅ Verification successful
Root POM version bump
The<version>fortracer-all-parenthas been updated from3.1.9to3.1.10. Ensure there are no lingering3.1.9references in the repository.Please run this script to verify that all POM files have been updated:
🏁 Script executed:
#!/bin/bash # Verify no remaining references to version 3.1.9 rg -n '<version>\s*3\.1\.9\s*</version>' || echo "No occurrences of 3.1.9 found."Length of output: 113
Root POM version bump verified — no remaining 3.1.9 references found
I ran the provided script and confirmed there are no lingering
<version>3.1.9</version>entries across all POMs. Everything looks up to date.tracer-extensions/pom.xml (1)
7-7: Approve parent version bump to 3.1.10
This aligns thetracer-extensionsmodule with the new release version.tracer-core/pom.xml (1)
7-7: Approve parent version bump to 3.1.10
Consistent version alignment for thetracer-coremodule as part of the 3.1.10 release.sofa-tracer-plugins/sofa-tracer-kafkamq-plugin/pom.xml (1)
8-8: Approve parent version bump to 3.1.10
Ensures the Kafka/MQ plugin module is updated for the 3.1.10 release.tracer-test/log4j2-test/pom.xml (1)
7-7: Approve parent version bump to 3.1.10
Updates the Log4j2 integration test module to the new project version.sofa-tracer-plugins/sofa-tracer-flexible-plugin/pom.xml (1)
8-8: Approve parent version bump to 3.1.10
Aligns the flexible plugin module with the overall project version.sofa-tracer-plugins/sofa-tracer-dubbo-2.6.x-plugin/pom.xml (1)
8-8: Consistent version bump to 3.1.10.
The parent project version has been updated correctly to align with the new release.sofa-tracer-plugins/sofa-tracer-dubbo-plugin/pom.xml (1)
8-8: Consistent version bump to 3.1.10.
The parent project version has been updated correctly to align with the new release.sofa-tracer-plugins/sofa-tracer-springmvc-plugin/pom.xml (1)
7-7: Consistent version bump to 3.1.10.
The parent project version has been updated correctly to align with the new release.sofa-tracer-plugins/sofa-tracer-rabbitmq-plugin/pom.xml (1)
8-8: Consistent version bump to 3.1.10.
The parent project version has been updated correctly to align with the new release.sofa-tracer-plugins/sofa-tracer-resttmplate-plugin/pom.xml (1)
8-8: Consistent version bump to 3.1.10.
The parent project version has been updated correctly to align with the new release.tracer-all/pom.xml (1)
8-8: Version update looks good.The version update from 3.1.9 to 3.1.10 is consistent between parent reference and artifact definition, which is appropriate for the new span event functionality being added.
Also applies to: 13-13
tracer-core/src/main/java/com/alipay/common/tracer/core/constants/SofaTracerConstant.java (1)
126-129: New constant for span event limit looks appropriate.The addition of
MAX_SPAN_EVENT_NUMwith a value of 100 establishes a reasonable limit for the number of events that can be associated with a span. This constant will help prevent excessive memory usage by limiting event collection per span.tracer-core/src/main/java/com/alipay/common/tracer/core/span/SpanEventData.java (2)
1-31: License and class documentation look good.The copyright notice, license, and class documentation follow the project's standards. The class has appropriate authorship attribution and version information.
32-41: Thread-safe data structure implementation is appropriate.Good choice using
ConcurrentHashMapfor the event tag storage, as this class will likely be accessed from multiple threads. The separation of tags by data type (string, number, boolean) is a clean design that prevents type casting issues.tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/type/TracerTestLogEnum.java (1)
30-33: Enum constant for client event logging looks good.The addition of
RPC_CLIENT_EVENTaligns with the new span event reporting functionality. The naming is consistent with the existing pattern and provides appropriate configuration for log files.tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/SofaTracerTest.java (5)
25-31: Import additions align with the span event implementation.The new imports appropriately include the necessary classes for event reporting functionality.
149-158: Event data added to span in testReportSpan looks good.The span event data addition is implemented correctly, with proper timestamp setting and tag addition.
186-190: Event data added to span in testIsDisableAllDigestLog looks good.The implementation properly adds event data with numeric and boolean tags.
209-212: Event data added to span in testIsDisableClientDigestLog looks good.The implementation properly adds event data with string tags.
229-232: Event data added to span in testClose looks good.The implementation properly adds event data with string tags after tracer closure, which is a good test case to verify behavior in edge conditions.
tracer-core/src/test/java/com/alipay/common/tracer/core/span/SofaTracerSpanTest.java (2)
25-40: Import changes and new constants look good.The import changes and new constants for client and server event log file types are appropriate.
72-81: Event reporter configuration in setup looks good.The event reporter setup is implemented correctly, configuring both client and server event reporters with the appropriate encoders.
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (2)
77-78: New eventData field for span events looks good.The addition of the SpanEventData field provides the necessary structure to support span events.
580-592: Event data getter and setter look good.The getter and setter methods for the eventData field are properly implemented.
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (6)
1-39: Code style and documentation look good.The class header is appropriately documented with license information and Javadoc. The class version is marked for the year 2025, which aligns with future-dated development.
40-53: Class structure and field declarations are appropriate.The class properly extends AbstractDiskReporter and defines the necessary fields with clear naming conventions. Using AtomicBoolean for initialization flag ensures thread safety.
73-92: Reporter type methods implementation is correct.Both getDigestReporterType and getStatReporterType methods are correctly implemented, returning eventLogType and empty string respectively, following the pattern in AbstractDiskReporter.
93-113: Lazy initialization in digestReport is thread-safe.The implementation uses a double-checked locking pattern with AtomicBoolean for thread safety during lazy initialization. The method correctly delegates to the AsyncCommonDigestAppenderManager for appending spans.
124-157: Getters are correctly implemented.All getter methods are properly implemented for accessing the class fields.
158-187: Thread-safe initialization of the log file.The initDigestFile method correctly implements synchronization for thread safety. It also handles dynamic configuration changes via logNameKey properly. The method follows the same pattern used in other reporters for consistency.
tracer-core/src/test/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporterTest.java (6)
1-43: Test class header and imports are appropriate.The imports and license header are properly structured. The class version is marked for 2025, which aligns with the implementation class.
44-68: Test setup is well implemented.The test class extends AbstractTestBase and properly sets up test objects. The mock for SofaTracerSpan is correctly created in the @before method.
69-79: Basic functionality tests are implemented correctly.The tests for getDigestReporterType and digestReport verify core functionality of the SpanEventDiskReporter class.
80-103: Getter tests are comprehensive.The test methods verify all getters of the SpanEventDiskReporter class, ensuring they return expected values.
104-124: Concurrency test for file initialization is thorough.The testFixInitDigestFile method properly tests concurrent initialization using multiple threads, ensuring that initialization is thread-safe by verifying there's only one initialization log entry. This is a critical test for ensuring thread safety.
125-155: Worker thread implementation is correct.The WorkerInitThread inner class correctly implements Runnable and creates a proper test SofaTracerSpan for testing concurrent reporter initialization.
tracer-core/src/main/java/com/alipay/common/tracer/core/SofaTracer.java (10)
75-78: New event reporter fields added.The new fields for clientEventReporter and serverEventReporter follow the same pattern as existing reporter fields and are correctly declared as final.
89-100: Existing constructor updated.The existing constructor has been updated to initialize the new event reporter fields to null, maintaining backward compatibility.
102-109: Sampler constructor updated.The sampler-only constructor has been updated to initialize the new event reporter fields to null, maintaining backward compatibility.
111-123: New constructor added for event reporters.A new constructor has been added that accepts all reporters, including the new event reporters. This follows the same pattern as the existing constructor.
174-197: reportEvent method implementation.The new reportEvent method mirrors the existing reportSpan method, providing consistent behavior for event reporting. It correctly handles sampling, listener invocation, and routing to the appropriate reporter based on span type.
209-217: close method updated.The close method has been updated to properly close the new event reporters if they are non-null.
235-242: Getter methods added for event reporters.New getter methods have been added for the event reporters, following the same pattern as existing getter methods.
462-465: Builder fields added for event reporters.New fields have been added to the Builder class for the event reporters, following the same pattern as existing reporter fields.
485-494: Builder methods added for event reporters.New fluent setter methods have been added to the Builder class for the event reporters, following the same pattern as existing reporter methods.
548-550: Build method updated.The build method has been updated to pass the new event reporters to the SofaTracer constructor.
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SpanEventData.java
Show resolved
Hide resolved
tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/SofaTracerTest.java
Show resolved
Hide resolved
tracer-core/src/test/java/com/alipay/common/tracer/core/span/SofaTracerSpanTest.java
Show resolved
Hide resolved
...src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java
Show resolved
Hide resolved
...src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (6)
61-71: Consider validating all critical parameters.While you've added validation for
eventLogTypeandcontextEncoder, consider also validatingeventRollingPolicywhich appears to be a required parameter for proper functioning.public SpanEventDiskReporter(String eventLogType, String eventRollingPolicy, String eventLogReserveConfig, SpanEncoder contextEncoder, String logNameKey) { AssertUtils.hasText(eventLogType, "eventLogType can't be empty"); AssertUtils.notNull(contextEncoder, "contextEncoder can't be null"); + AssertUtils.hasText(eventRollingPolicy, "eventRollingPolicy can't be empty"); this.eventLogType = eventLogType; this.eventRollingPolicy = eventRollingPolicy; this.eventLogReserveConfig = eventLogReserveConfig; this.contextEncoder = contextEncoder; this.logNameKey = logNameKey; }
119-122: Simplify empty method.The
return;statement is unnecessary in a void method. Consider removing it for cleaner code.@Override public void statisticReport(SofaTracerSpan span) { - return; }
78-91: Improve method documentation for override methods.The method Javadoc for
getDigestReporterType()andgetStatReporterType()is incomplete. Consider improving the documentation to clearly explain the purpose and return value of these methods./** - * Get digest reporter instance type + * Get digest reporter instance type. + * This returns the event log type that this reporter handles. * - * @return + * @return the event log type for this reporter */
158-186: Consider extracting logging behavior to separate interface.Based on the previous comment by EvenLjj (line 39), consider whether event reporting should be abstracted into a separate interface, similar to how
SofaTracerStatisticReporterhandles statistics. This would provide better separation of concerns and potentially simplify future extensions.
100-112: Refactor duplicate appender check logic.The check for appender and encoder existence appears in both
digestReport()andinitDigestFile(). Consider extracting this check into a private method to avoid duplicated logic and improve maintainability.private boolean isAppenderAndEncoderExist() { AsyncCommonDigestAppenderManager asyncDigestManager = SofaTracerDigestReporterAsyncManager .getSofaTracerDigestReporterAsyncManager(); return asyncDigestManager.isAppenderAndEncoderExist(this.eventLogType); } @Override public void digestReport(SofaTracerSpan span) { //lazy initialization if (!this.isEventFileInited.get()) { this.initDigestFile(); } AsyncCommonDigestAppenderManager asyncDigestManager = SofaTracerDigestReporterAsyncManager .getSofaTracerDigestReporterAsyncManager(); - if (asyncDigestManager.isAppenderAndEncoderExist(this.eventLogType)) { + if (isAppenderAndEncoderExist()) { //Print only when appender and encoder are present asyncDigestManager.append(span); } else { SelfLog.warn(span.toString() + " have no logType set, so ignore data persistence."); } }
158-186: Add Javadoc for initDigestFile method.The
initDigestFile()method lacks Javadoc. Consider adding documentation to explain its purpose, especially since it handles critical initialization logic.+/** + * Initializes the digest file for event logging. + * This method is synchronized and uses double-check locking to ensure thread safety. + * It creates and registers the file appender with the async manager. + */ private synchronized void initDigestFile() { // Method implementation... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java(1 hunks)tracer-core/src/main/java/com/alipay/common/tracer/core/span/SpanEventData.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tracer-core/src/main/java/com/alipay/common/tracer/core/span/SpanEventData.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (1)
64-65: Added parameter validation addresses previous review feedback.The constructor now properly validates both
eventLogTypeandcontextEncoderparameters, addressing the previous review comment.
|
|
||
| private final String eventRollingPolicy; | ||
|
|
||
| private String eventLogReserveConfig; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider thread safety for non-final field.
The eventLogReserveConfig field is not final and is modified in the initDigestFile() method. Since this class operates in a multi-threaded environment (as evidenced by the synchronization and atomic boolean), consider ensuring proper visibility for this field, possibly by making it volatile.
- private String eventLogReserveConfig;
+ private volatile String eventLogReserveConfig;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private String eventLogReserveConfig; | |
| private volatile String eventLogReserveConfig; |
Motivation:
Explain the context, and why you're making that change.
To make others understand what is the problem you're trying to solve.
Modification:
Describe the idea and modifications you've done.
Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores