support event #556
support event #556
Conversation
|
Hi @wujiaming88, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
WalkthroughThis pull request updates the version numbers in multiple Maven POM files from 3.1.9 to 3.1.10-SNAPSHOT for both parent and individual artifacts. Additionally, it introduces event reporting capabilities by adding new reporter fields and methods to the core tracing components, a new disk reporter class for span events, supplementary changes to span tagging and reporting, and corresponding tests to validate these enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant Span as SofaTracerSpan
participant Tracer as SofaTracer
participant Reporter as SpanEventDiskReporter
participant Appender as AsyncCommonDigestAppenderManager
Span->>Tracer: reportEvent(span)
alt Client Event
Tracer->>Reporter: clientEventReporter.report(span)
else Server Event
Tracer->>Reporter: serverEventReporter.report(span)
end
Reporter->>Appender: Append span event to disk
Possibly related PRs
Suggested labels
Suggested reviewers
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: 5
🧹 Nitpick comments (3)
tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/SofaTracerTest.java (1)
79-86: Good implementation of event reportersThe initialization of both client and server event reporters follows the pattern of the existing reporter initialization. However, there are a few things to note:
- The server event reporter is using
ClientSpanEventEncoder, which might be intentional, but consider if a separateServerSpanEventEncoderwould be more appropriate.- Both reporters are initialized with empty strings for rolling policy and log reserve config, although the corresponding values are available from
TracerTestLogEnum.Consider using the enum's rolling policy and log reserve config:
-SpanEventDiskReporter clientEventReporter = new SpanEventDiskReporter(TracerTestLogEnum.RPC_CLIENT_EVENT.getDefaultLogName(), "", "", new ClientSpanEventEncoder(), null); +SpanEventDiskReporter clientEventReporter = new SpanEventDiskReporter(TracerTestLogEnum.RPC_CLIENT_EVENT.getDefaultLogName(), + TracerTestLogEnum.RPC_CLIENT_EVENT.getRollingKey(), + TracerTestLogEnum.RPC_CLIENT_EVENT.getLogReverseKey(), + new ClientSpanEventEncoder(), null); -SpanEventDiskReporter serverEventReporter = new SpanEventDiskReporter(TracerTestLogEnum.RPC_SERVER.getDefaultLogName(), "", "", new ClientSpanEventEncoder(), null); +SpanEventDiskReporter serverEventReporter = new SpanEventDiskReporter(TracerTestLogEnum.RPC_SERVER.getDefaultLogName(), + TracerTestLogEnum.RPC_SERVER.getRollingKey(), + TracerTestLogEnum.RPC_SERVER.getLogReverseKey(), + new ClientSpanEventEncoder(), null);tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java (1)
27-61: Well-structured event encoding implementationThe
encodemethod logically handles each component of the span event:
- Timestamp
- Trace and Span IDs
- String tags
- Boolean tags
- Numeric tags
- Baggage
However, there are a few potential improvements:
- The empty string builders for boolean and numeric tags could result in trailing
&characters if there are no entries- There's no null checking or empty map handling for the collections
Consider adding defensive coding to handle empty maps:
// tags bool Map<String, Boolean> tagsBool = span.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); +if (!tagsBool.isEmpty()) { + for (Map.Entry<String, Boolean> entry : tagsBool.entrySet()) { + tagsBoolBuild.append(entry.getKey()).append(StringUtils.EQUAL) + .append(entry.getValue().toString()).append(StringUtils.AND); + } + // Remove trailing '&' if present + if (tagsBoolBuild.length() > 0 && tagsBoolBuild.charAt(tagsBoolBuild.length() - 1) == '&') { + tagsBoolBuild.setLength(tagsBoolBuild.length() - 1); + } } xsb.append(tagsBoolBuild.toString()); // tags number Map<String, Number> tagsNum = span.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); +if (!tagsNum.isEmpty()) { + for (Map.Entry<String, Number> entry : tagsNum.entrySet()) { + tagsNumBuild.append(entry.getKey()).append(StringUtils.EQUAL) + .append(entry.getValue().toString()).append(StringUtils.AND); + } + // Remove trailing '&' if present + if (tagsNumBuild.length() > 0 && tagsNumBuild.charAt(tagsNumBuild.length() - 1) == '&') { + tagsNumBuild.setLength(tagsNumBuild.length() - 1); + } } xsb.append(tagsNumBuild.toString());tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (1)
109-112: Empty statisticReport method.The statisticReport method is overridden but doesn't perform any actions. It would be helpful to include a comment explaining why the method is empty.
/** * print stat log * * @param span span */ @Override public void statisticReport(SofaTracerSpan span) { + // This reporter doesn't support statistical reporting, so this method intentionally does nothing return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
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(9 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(14 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(24 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 Definitions (6)
tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/SofaTracerTest.java (1)
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (1) (1)
SofaTracerSpan(50-707)
tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java (1)
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (1) (1)
SofaTracerSpan(50-707)
tracer-core/src/test/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporterTest.java (3)
tracer-core/src/main/java/com/alipay/common/tracer/core/SofaTracer.java (1) (1)
SofaTracer(57-555)tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (1) (1)
SofaTracerSpan(50-707)tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java (1) (1)
ClientSpanEventEncoder(23-62)
tracer-core/src/test/java/com/alipay/common/tracer/core/span/SofaTracerSpanTest.java (3)
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (1) (1)
SpanEventDiskReporter(28-177)tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java (1) (1)
ClientSpanEventEncoder(23-62)tracer-core/src/main/java/com/alipay/common/tracer/core/context/span/SofaTracerSpanContext.java (1) (1)
SofaTracerSpanContext(37-451)
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (1)
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (1) (1)
SofaTracerSpan(50-707)
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (4)
tracer-core/src/main/java/com/alipay/common/tracer/core/utils/StringUtils.java (1) (1)
StringUtils(36-740)tracer-core/src/main/java/com/alipay/common/tracer/core/context/span/SofaTracerSpanContext.java (1) (1)
SofaTracerSpanContext(37-451)tracer-core/src/main/java/com/alipay/common/tracer/core/appender/self/SelfLog.java (1) (1)
SelfLog(30-153)tracer-core/src/main/java/com/alipay/common/tracer/core/constants/SofaTracerConstant.java (1) (1)
SofaTracerConstant(29-125)
🔇 Additional comments (60)
tracer-test/logback-test/pom.xml (1)
7-7: Parent Version Update for Logback Test Module
The parent's version is updated to<version>3.1.10-SNAPSHOT</version>, which aligns with the new development iteration. Make sure that this update is coordinated with other modules and that there are no incompatibility issues with the dependency management.tracer-extensions/pom.xml (1)
7-7: Verify Updated Parent Version in Tracer Extensions
The<version>element under the<parent>section has been updated to3.1.10-SNAPSHOT, bringing it in line with other modules. Confirm that all inherited settings and dependency versions from the parent POM remain consistent and do not introduce conflicts.sofa-tracer-plugins/sofa-tracer-springmvc-plugin/pom.xml (1)
7-7: Ensure Consistency of Parent Version in SpringMVC Plugin
The parent's version change to3.1.10-SNAPSHOTis consistently applied. Verify that any module-specific configurations that might depend on the parent version are still valid, especially regarding relative paths and dependency management.tracer-test/log4j-test/pom.xml (1)
7-7: Update Parent Version in Log4j Test Module
The update to<version>3.1.10-SNAPSHOT</version>for the parent artifact is applied correctly. Ensure that this change does not adversely affect the test dependencies and that all modules consuming this configuration are aligned with the new snapshot release.tracer-test/log4j2-test/pom.xml (1)
7-7: Parent Version Alignment in Log4j2 Test Module
The parent's version field is updated to3.1.10-SNAPSHOT, matching the broader transition across the project. It would be useful to double-check that the test scope dependencies and plugin configurations (such as for Log4j2) remain compatible with the new parent version.sofa-tracer-plugins/sofa-tracer-kafkamq-plugin/pom.xml (1)
8-8: Parent Version Update Confirmed
The parent version has been updated to 3.1.10-SNAPSHOT, which aligns with the new development snapshot across the project.sofa-tracer-plugins/sofa-tracer-rocketmq-plugin/pom.xml (1)
8-8: Parent Version Synchronization
The parent version is correctly updated to 3.1.10-SNAPSHOT, ensuring consistency with other plugins and modules.tracer-test/core-test/pom.xml (1)
4-9: Parent Version Update in Test Module
The parent version within this test project has been updated to 3.1.10-SNAPSHOT, which maintains consistency across the project. Verify that all dependent test configurations adjust accordingly.sofa-tracer-plugins/sofa-tracer-mongodb-plugin/pom.xml (1)
8-8: Consistent Version Bump
The parent version is updated to 3.1.10-SNAPSHOT, ensuring uniformity across the plugin modules.sofa-tracer-plugins/sofa-tracer-dubbo-2.6.x-plugin/pom.xml (1)
8-8: Parent Version Alignment
The parent version has been successfully bumped to 3.1.10-SNAPSHOT, matching the version updates in other parts of the project.tracer-sofa-boot-starter/pom.xml (1)
8-8: Parent Version Update Confirmed.
The parent version is now set to<version>3.1.10-SNAPSHOT</version>, which aligns with the project's current development phase. Please verify that all modules referencing this parent are compatible with the snapshot update.sofa-tracer-plugins/sofa-tracer-dubbo-common-plugin/pom.xml (1)
8-8: Parent Version Updated for Dubbo Common Plugin.
The updated version<version>3.1.10-SNAPSHOT</version>in the parent section ensures this module is aligned with the overall project versioning. Confirm that this change does not introduce conflicts with any referenced dependencies.sofa-tracer-plugins/sofa-tracer-zipkin-plugin/pom.xml (1)
8-8: Consistent Parent Version in Zipkin Plugin.
The parent version update to<version>3.1.10-SNAPSHOT</version>is correctly applied. It maintains consistency with other modules, which is essential for coordinated builds.sofa-tracer-plugins/sofa-tracer-spring-cloud-plugin/pom.xml (1)
8-8: Aligned Parent Version for Spring Cloud Plugin.
By updating the parent version to<version>3.1.10-SNAPSHOT</version>, this module remains in sync with the most recent development iteration. Ensure that any Spring Cloud related configurations are verified against this snapshot version.sofa-tracer-plugins/sofa-tracer-flexible-plugin/pom.xml (1)
8-8: Snapshot Version Update for Flexible Plugin.
The parent tag now reflects<version>3.1.10-SNAPSHOT</version>, providing consistency across modules. It is recommended to run integration tests to ensure that the snapshot dependencies do not introduce unexpected behaviors.sofa-tracer-plugins/sofa-tracer-dubbo-plugin/pom.xml (1)
8-8: Version Update Verified.
The parent version has been updated to<version>3.1.10-SNAPSHOT</version>, which aligns this module with the new development snapshot. This change is consistent with similar updates across the project.sofa-tracer-plugins/sofa-tracer-resttmplate-plugin/pom.xml (1)
8-8: Parent Version Update Approved.
The update of the parent artifact version to<version>3.1.10-SNAPSHOT</version>is correctly applied here. Please ensure that all dependency and plugin configurations remain compatible with this new snapshot.sofa-tracer-plugins/sofa-tracer-springmessage-plugin/pom.xml (1)
8-8: Consistent Parent Version Update.
The change to<version>3.1.10-SNAPSHOT</version>in the parent configuration is in line with the updates made in other modules. No issues were detected.sofa-tracer-plugins/sofa-tracer-datasource-plugin/pom.xml (1)
8-8: Parent Version Correctly Updated.
The parent version is now set to<version>3.1.10-SNAPSHOT</version>, ensuring that this plugin is consistent with the project-wide version bump.pom.xml (1)
12-12: Project Version Update Confirmed.
The overall project version has been updated to<version>3.1.10-SNAPSHOT</version>, ensuring consistency across dependency management and module versioning. Please verify that all modules correctly reference this updated version.sofa-tracer-plugins/sofa-tracer-httpclient-plugin/pom.xml (1)
8-8: Parent Version Update Applied Correctly.The parent version is updated to
3.1.10-SNAPSHOT, ensuring consistency with the overall project version upgrade. Please verify that this change aligns with dependency management across related modules.sofa-tracer-plugins/sofa-tracer-redis-plugin/pom.xml (1)
8-8: Consistent Parent Version Update.The switch from
3.1.9to3.1.10-SNAPSHOTis properly applied here. Confirm that no downstream modules are adversely impacted by this version change.tracer-all/pom.xml (2)
5-9: Parent POM Version Synchronization.The parent version is updated to
3.1.10-SNAPSHOT, aligning with the recent project-wide version bump. This ensures that all inherited properties and dependency management are current.
12-13: Artifact Version Update Confirmed.Updating the artifact’s own version to
3.1.10-SNAPSHOTmaintains cohesion with the parent version. This change is straightforward and consistent with the release strategy.sofa-tracer-plugins/sofa-tracer-rabbitmq-plugin/pom.xml (1)
8-8: Parent Version Update is Consistent.The update to
3.1.10-SNAPSHOTfor the parent artifact is correctly implemented. Make sure that relative paths and any optional configurations remain properly referenced.tracer-core/pom.xml (1)
7-7: Parent Version Update Propagated.The update in
tracer-core/pom.xmlto3.1.10-SNAPSHOTensures that the core module inherits the latest settings and dependency versions from the parent POM. Confirm that any modules relying on these core components are tested for compatibility.sofa-tracer-plugins/sofa-tracer-okhttp-plugin/pom.xml (1)
8-8: Update Parent Version to 3.1.10-SNAPSHOTThe parent version has been correctly updated from the previous release version to
3.1.10-SNAPSHOT, aligning this module with the overall release cadence. Please ensure that all related modules are updated consistently.tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/type/TracerTestLogEnum.java (1)
27-32: Good addition of the RPC_CLIENT_EVENT constantThe new
RPC_CLIENT_EVENTenum constant follows the existing pattern and naming convention consistently. The reformatting of theRPC_SERVERconstant also improves readability while maintaining functionality.tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/SofaTracerTest.java (7)
25-25: Appropriate import for event reporting functionalityThe addition of the
SpanEventDiskReporterimport aligns with the new event support functionality being added in this PR.
31-31: Appropriate import for event encoding functionalityThe addition of the
ClientSpanEventEncoderimport supports the new event encoding functionality.
90-92: Well-structured builder integrationThe addition of client and server event reporters to the
SofaTracer.Builderconfiguration is clean and maintains the fluent builder pattern.
145-145: Appropriate test for event reportingThe addition of
span.reportEvent()to thetestReportSpanmethod allows testing of the event reporting feature alongside the regular span reporting.
173-173: Consistent testing approachIncluding
reportEvent()in thetestIsDisableAllDigestLogtest ensures event reporting is tested when digest logs are disabled.
193-193: Thorough testing of event reportingAdding
reportEvent()to thetestIsDisableClientDigestLogmethod ensures event reporting is tested when client digest logs are disabled.
211-211: Complete coverage of event reporting scenariosThe addition of
reportEvent()to thetestClosemethod ensures event reporting is properly tested after the tracer is closed.tracer-core/src/test/java/com/alipay/common/tracer/core/tracertest/encoder/ClientSpanEventEncoder.java (1)
23-26: Good implementation of the SpanEncoder interfaceThe class properly implements the
SpanEncoder<SofaTracerSpan>interface, and the usage ofXStringBuilderfor building the encoded string is appropriate and consistent with other encoder implementations.tracer-core/src/test/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporterTest.java (4)
48-55: Well-structured test setupThe test setup method (
before) properly initializes theSpanEventDiskReporterinstance with the expected configuration parameters and a mockSofaTracerSpan. This is consistent with good testing practices.
57-90: Comprehensive testing of reporter functionalityThese test methods provide good coverage of the basic functionality of the
SpanEventDiskReporterclass, testing the log type, rolling policy, log reserve config, context encoder, and report digest functionality.
92-110: Good concurrent initialization testThe
testFixInitDigestFilemethod is a well-designed test for ensuring thread safety during file initialization. It:
- Creates a thread pool with a specific number of threads
- Uses a
CountDownLatchto synchronize the test- Verifies that only one log entry is created during concurrent initialization
This is particularly important for a reporter that might be accessed concurrently in production.
112-140: Well-implemented worker thread classThe
WorkerInitThreadinner class properly implements theRunnableinterface and provides a clear implementation for the concurrent test. It:
- Stores required references in the constructor
- Properly implements the
runmethod to process commands and count down the latch- Creates and configures a test span for reporting
This class design is clean and focused on its testing purpose.
tracer-core/src/test/java/com/alipay/common/tracer/core/span/SofaTracerSpanTest.java (4)
25-25: Clean imports for new event support feature.The imports for
SpanEventDiskReporterandClientSpanEventEncoderare correctly added to support the new event functionality being tested.Also applies to: 28-28
61-62: Well-structured constants for event log types.Constants for client and server event log types are properly defined, consistent with the existing coding style for log type constants.
Also applies to: 65-65
77-84: Good configuration of event reporters in the setup method.The test setup appropriately initializes client and server event reporters, integrating them into the tracer builder. This ensures comprehensive testing of the new event functionality.
207-229: Comprehensive test coverage for event tag functionality.The new test methods thoroughly validate the different types of event tags (string, number, boolean) that can be set on spans. Each test follows the proper assertion pattern, ensuring tags are correctly stored and retrieved.
tracer-core/src/main/java/com/alipay/common/tracer/core/span/SofaTracerSpan.java (4)
73-77: Well-designed event tag storage.The addition of map fields for event tags follows the same pattern as existing tag maps, providing consistent and type-safe storage for different event tag types.
190-200: Robust event reporting implementation.The
reportEvent()method properly checks if the span is closed before attempting to report an event. This prevents errors from reporting events on closed spans and provides appropriate error logging.
243-283: Well-implemented event tag methods.The three event tag methods (
setEventTagfor String, boolean, and Number) are implemented correctly with appropriate null/blank checks, consistent with the existing tag methods. Each method correctly returnsthisfor method chaining.
563-589: Comprehensive getter methods for event tags.The getter methods for event tags are well-implemented and follow the same pattern as the existing getter methods for regular tags, providing consistent access to the event tag maps.
tracer-core/src/main/java/com/alipay/common/tracer/core/SofaTracer.java (8)
79-81: Clear declaration of event reporter fields.The new event reporter fields are properly declared as final, consistent with the existing reporter fields, ensuring thread safety and indicating their immutability after initialization.
98-112: Proper initialization in existing constructors.The existing constructors correctly initialize the new event reporter fields to null, maintaining backward compatibility with code that uses these constructors.
115-126: Well-designed new constructor supporting event reporters.The new constructor properly initializes all fields, including the new event reporters, allowing clients to create a tracer with event reporting capability.
177-200: Consistent event reporting implementation.The
reportEventmethod follows the same pattern as the existingreportSpanmethod, providing a consistent approach to handling both regular spans and event spans.
213-219: Proper resource cleanup for event reporters.The
closemethod is correctly updated to close the event reporters when they exist, ensuring proper resource cleanup.
238-244: Appropriate getter methods for event reporters.The getter methods for event reporters follow the same pattern as the existing getter methods, providing consistent access to the reporter instances.
465-467: Builder pattern correctly extended for event reporters.The builder class is properly extended with methods for setting event reporters, maintaining the fluent interface pattern and making it easy to configure a tracer with event reporting capability.
Also applies to: 488-496
551-552: Updated build method to use new constructor.The build method correctly creates a new SofaTracer instance using the constructor that supports event reporters, ensuring that all configured components are properly initialized.
tracer-core/src/main/java/com/alipay/common/tracer/core/reporter/digest/event/SpanEventDiskReporter.java (4)
28-41: Well-structured class fields.The fields are appropriately declared as final where applicable, and the use of AtomicBoolean for thread-safe initialization is a good practice.
52-60: Good constructor implementation with proper validation.The constructor properly validates that the eventLogType is not empty using AssertUtils, which is a good practice to fail fast if invalid parameters are provided.
89-102: Properly implemented digestReport method with lazy initialization.The digestReport method uses lazy initialization of the event file, which is an efficient approach. It also properly checks if the appender and encoder exist before attempting to append the span.
148-176: Thread-safe file initialization.The
initDigestFilemethod is correctly synchronized to ensure thread safety during file initialization. The double-check pattern is also used correctly to avoid unnecessary synchronization.
| /** | ||
| * Ant Group | ||
| * Copyright (c) 2004-2025 All Rights Reserved. | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update the license header
The current header uses "Ant Group Copyright" which doesn't match the Apache License header used in other files in this project.
Replace with the standard Apache License header:
-/**
- * Ant Group
- * Copyright (c) 2004-2025 All Rights Reserved.
- */
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */📝 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.
| /** | |
| * Ant Group | |
| * Copyright (c) 2004-2025 All Rights Reserved. | |
| */ | |
| /* | |
| * Licensed to the Apache Software Foundation (ASF) under one or more | |
| * contributor license agreements. See the NOTICE file distributed with | |
| * this work for additional information regarding copyright ownership. | |
| * The ASF licenses this file to You under the Apache License, Version 2.0 | |
| * (the "License"); you may not use this file except in compliance with | |
| * the License. You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ |
| /** | ||
| * Ant Group | ||
| * Copyright (c) 2004-2025 All Rights Reserved. | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update the license header
Similar to the ClientSpanEventEncoder class, this file uses the "Ant Group Copyright" header rather than the Apache License header used in other project files.
Replace with the standard Apache License header:
-/**
- * Ant Group
- * Copyright (c) 2004-2025 All Rights Reserved.
- */
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */📝 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.
| /** | |
| * Ant Group | |
| * Copyright (c) 2004-2025 All Rights Reserved. | |
| */ | |
| /* | |
| * Licensed to the Apache Software Foundation (ASF) under one or more | |
| * contributor license agreements. See the NOTICE file distributed with | |
| * this work for additional information regarding copyright ownership. | |
| * The ASF licenses this file to You under the Apache License, Version 2.0 | |
| * (the "License"); you may not use this file except in compliance with | |
| * the License. You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ |
| @Test | ||
| public void testEvent() { | ||
| SofaTracerSpan span = (SofaTracerSpan) this.sofaTracer.buildSpan("testWithTimestamp") | ||
| .withStartTimestamp(111).start(); | ||
| span.setEventTag("tag.key", "value"); | ||
| span.setTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT); | ||
| span.reportEvent(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing assertions in event reporting test.
The testEvent() method doesn't include any assertions to verify that the event was properly reported or processed. This makes it difficult to determine if the test successfully validates the functionality.
Consider adding assertions to verify the event was successfully reported, perhaps by checking log output or using a mock reporter that can track reported events:
@Test
public void testEvent() {
SofaTracerSpan span = (SofaTracerSpan) this.sofaTracer.buildSpan("testWithTimestamp")
.withStartTimestamp(111).start();
span.setEventTag("tag.key", "value");
span.setTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT);
span.reportEvent();
+
+ // Verify event was reported correctly
+ // Option 1: If using a mock reporter, verify it was called
+ // Option 2: Check if event data was properly logged
+ assertTrue("Event should be reported", /* verification logic */);
}📝 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.
| @Test | |
| public void testEvent() { | |
| SofaTracerSpan span = (SofaTracerSpan) this.sofaTracer.buildSpan("testWithTimestamp") | |
| .withStartTimestamp(111).start(); | |
| span.setEventTag("tag.key", "value"); | |
| span.setTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT); | |
| span.reportEvent(); | |
| } | |
| @Test | |
| public void testEvent() { | |
| SofaTracerSpan span = (SofaTracerSpan) this.sofaTracer.buildSpan("testWithTimestamp") | |
| .withStartTimestamp(111).start(); | |
| span.setEventTag("tag.key", "value"); | |
| span.setTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT); | |
| span.reportEvent(); | |
| // Verify event was reported correctly | |
| // Option 1: If using a mock reporter, verify it was called | |
| // Option 2: Check if event data was properly logged | |
| assertTrue("Event should be reported", /* verification logic */); | |
| } |
| /** | ||
| * Ant Group | ||
| * Copyright (c) 2004-2025 All Rights Reserved. | ||
| */ |
There was a problem hiding this comment.
Inconsistent license header.
The copyright notice "Ant Group" differs from the Apache Software Foundation license used in other project files. This could cause confusion about licensing and ownership.
Update the license header to match the Apache Software Foundation license used in other files:
-/**
- * Ant Group
- * Copyright (c) 2004-2025 All Rights Reserved.
- */
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */📝 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.
| /** | |
| * Ant Group | |
| * Copyright (c) 2004-2025 All Rights Reserved. | |
| */ | |
| /* | |
| * Licensed to the Apache Software Foundation (ASF) under one or more | |
| * contributor license agreements. See the NOTICE file distributed with | |
| * this work for additional information regarding copyright ownership. | |
| * The ASF licenses this file to You under the Apache License, Version 2.0 | |
| * (the "License"); you may not use this file except in compliance with | |
| * the License. You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ |
| if (StringUtils.isNotBlank(logNameKey)) { | ||
| String currentDigestLogReserveConfig = SofaTracerConfiguration | ||
| .getLogReserveConfig(logNameKey); | ||
| if (!currentDigestLogReserveConfig.equals(eventLogReserveConfig)) { | ||
| SelfLog.info("the lognamekey : " + logNameKey | ||
| + " take effect. the old logreserveconfig is " | ||
| + eventLogReserveConfig + " and " + "the new logreverseconfig is " | ||
| + currentDigestLogReserveConfig); | ||
| eventLogReserveConfig = currentDigestLogReserveConfig; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential null pointer exception in log reserve config handling.
The code checks if logNameKey is not blank and then calls SofaTracerConfiguration.getLogReserveConfig(logNameKey), but there's no null check for the returned value before using equals().
if (StringUtils.isNotBlank(logNameKey)) {
String currentDigestLogReserveConfig = SofaTracerConfiguration
.getLogReserveConfig(logNameKey);
- if (!currentDigestLogReserveConfig.equals(eventLogReserveConfig)) {
+ if (currentDigestLogReserveConfig != null && !currentDigestLogReserveConfig.equals(eventLogReserveConfig)) {
SelfLog.info("the lognamekey : " + logNameKey
+ " take effect. the old logreserveconfig is "
+ eventLogReserveConfig + " and " + "the new logreverseconfig is "
+ currentDigestLogReserveConfig);
eventLogReserveConfig = currentDigestLogReserveConfig;
}
}📝 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.
| if (StringUtils.isNotBlank(logNameKey)) { | |
| String currentDigestLogReserveConfig = SofaTracerConfiguration | |
| .getLogReserveConfig(logNameKey); | |
| if (!currentDigestLogReserveConfig.equals(eventLogReserveConfig)) { | |
| SelfLog.info("the lognamekey : " + logNameKey | |
| + " take effect. the old logreserveconfig is " | |
| + eventLogReserveConfig + " and " + "the new logreverseconfig is " | |
| + currentDigestLogReserveConfig); | |
| eventLogReserveConfig = currentDigestLogReserveConfig; | |
| } | |
| } | |
| if (StringUtils.isNotBlank(logNameKey)) { | |
| String currentDigestLogReserveConfig = SofaTracerConfiguration | |
| .getLogReserveConfig(logNameKey); | |
| if (currentDigestLogReserveConfig != null && !currentDigestLogReserveConfig.equals(eventLogReserveConfig)) { | |
| SelfLog.info("the lognamekey : " + logNameKey | |
| + " take effect. the old logreserveconfig is " | |
| + eventLogReserveConfig + " and " + "the new logreverseconfig is " | |
| + currentDigestLogReserveConfig); | |
| eventLogReserveConfig = currentDigestLogReserveConfig; | |
| } | |
| } |
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
Chores
Tests