fix: replaced the stopwatch implementation with micrometer #37200
Conversation
WalkthroughThe changes in this pull request involve the addition of new constants to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-git/src/main/java/com/appsmith/git/constants/CommonConstants.java (2)
39-42: LGTM! Consider grouping related constants.The new constants follow good naming conventions and align with the migration to Micrometer. Consider grouping these Git metrics-related constants together at the top of the file with other Git-related constants for better organization.
public class CommonConstants { + // Git Metrics Constants + public static final String GIT_SAVE_ARTIFACT = "git.saveArtifact"; + public static final String TIME_TAKEN_TO_SAVE_ARTIFACT = "Time taken to save Artifact"; + public static final String GIT_DESERIALIZE_ARTIFACT = "git.deserializeArtifact"; + public static final String TIME_TAKEN_TO_DESERIALIZE_ARTIFACT = "Time taken to deserialize Artifact from Git repo"; + // This field will be useful when we migrate fields within JSON files... public static Integer fileFormatVersion = 5; // ... rest of the constants
39-42: Add documentation for metrics usage.Consider adding Javadoc comments to explain the purpose and usage context of these metrics constants, especially since they're part of a monitoring implementation change.
+ /** + * Constants for Micrometer metrics tracking Git operations. + * These are used to measure performance of artifact operations in the Git integration. + */ public static final String GIT_SAVE_ARTIFACT = "git.saveArtifact"; public static final String TIME_TAKEN_TO_SAVE_ARTIFACT = "Time taken to save Artifact"; public static final String GIT_DESERIALIZE_ARTIFACT = "git.deserializeArtifact"; public static final String TIME_TAKEN_TO_DESERIALIZE_ARTIFACT = "Time taken to deserialize Artifact from Git repo";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-git/src/main/java/com/appsmith/git/constants/CommonConstants.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(6 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
130-133:
Capture and use the execution time from sample.stop(...)
Instead of calling sample.toString(), capture the execution time when stopping the timer sample.
Apply this diff to fix the issue:
sample.stop(Timer.builder(CommonConstants.GIT_SAVE_ARTIFACT)
.description(CommonConstants.TIME_TAKEN_TO_SAVE_ARTIFACT)
.register(meterRegistry));
-observationHelper.endSpan(span, true);
+"executionTime", sample.stop(...));
+observationHelper.endSpan(span, true);Likely invalid or redundant comment.
223-226:
Capture and use execution time from timer sample
Ensure you capture the execution time when stopping the timer and use it accordingly.
Apply this diff:
sample.stop(Timer.builder(CommonConstants.GIT_DESERIALIZE_ARTIFACT)
.description(CommonConstants.TIME_TAKEN_TO_DESERIALIZE_ARTIFACT)
.register(meterRegistry));
-observationHelper.endSpan(span, true);
+"executionTime", sample.stop(...));
+observationHelper.endSpan(span, true);Likely invalid or redundant comment.
| stopwatch.getFlow(), | ||
| "executionTime", | ||
| stopwatch.getExecutionTime()); | ||
| "executionTime", sample.toString()); |
There was a problem hiding this comment.
Replace sample.toString() with actual execution time
Use the execution time captured from the timer sample instead of calling sample.toString().
Apply this diff:
-"executionTime", sample.toString());
+"executionTime", /* captured execution time variable */);Committable suggestion skipped: line range outside the PR's diff.
| @@ -141,6 +148,7 @@ public Mono<Path> saveArtifactToLocalRepoWithAnalytics( | |||
| }); | |||
| } catch (IOException | GitAPIException e) { | |||
| log.error("Error occurred while saving files to local git repo: ", e); | |||
| observationHelper.endSpan(span, false); | |||
There was a problem hiding this comment.
Ensure span is ended even if an exception occurs
The span should be ended in a finally block to guarantee it's closed regardless of exceptions.
Apply this diff:
try {
// Existing code
} catch (IOException | GitAPIException e) {
log.error("Error occurred while saving files to local git repo: ", e);
- observationHelper.endSpan(span, false);
throw Exceptions.propagate(e);
+} finally {
+ observationHelper.endSpan(span, false);
}Committable suggestion skipped: line range outside the PR's diff.
| stopwatch.getFlow(), | ||
| "executionTime", | ||
| stopwatch.getExecutionTime()); | ||
| "executionTime", sample.toString()); |
There was a problem hiding this comment.
Use captured execution time instead of sample.toString()
Replace sample.toString() with the actual execution time captured from the timer.
Apply this diff:
-"executionTime", sample.toString());
+"executionTime", /* captured execution time variable */);Committable suggestion skipped: line range outside the PR's diff.
| }) | ||
| .doOnError(e -> { | ||
| observationHelper.endSpan(span, false); | ||
| log.error("Error deserializing artifact : {}", e.getMessage()); |
There was a problem hiding this comment.
End the span in a doFinally block
Ensure the span is ended even if an error occurs by using doFinally instead of doOnError.
Apply this diff:
-.doOnError(e -> {
- observationHelper.endSpan(span, false);
- log.error("Error deserializing artifact : {}", e.getMessage());
-});
+.doFinally(signalType -> {
+ boolean success = !signalType.equals(SignalType.ON_ERROR);
+ observationHelper.endSpan(span, success);
+});📝 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.
| }) | |
| .doOnError(e -> { | |
| observationHelper.endSpan(span, false); | |
| log.error("Error deserializing artifact : {}", e.getMessage()); | |
| }) | |
| .doFinally(signalType -> { | |
| boolean success = !signalType.equals(SignalType.ON_ERROR); | |
| observationHelper.endSpan(span, success); | |
| }); |
| Timer.Sample sample = Timer.start(meterRegistry); | ||
| Span span = observationHelper.createSpan(AnalyticsEvents.GIT_DESERIALIZE_APP_RESOURCES_FROM_FILE.getEventName()); | ||
| observationHelper.startSpan(span, true); |
There was a problem hiding this comment.
Activate span in the current context
Use try-with-resources to manage span scope correctly during deserialization.
Apply this diff:
Timer.Sample sample = Timer.start(meterRegistry);
Span span = observationHelper.createSpan(AnalyticsEvents.GIT_DESERIALIZE_APP_RESOURCES_FROM_FILE.getEventName());
-observationHelper.startSpan(span, true);
+try (Tracer.SpanInScope scope = tracer.withSpan(span)) {
// Existing code inside the try block
+}Committable suggestion skipped: line range outside the PR's diff.
| Timer.Sample sample = Timer.start(meterRegistry); | ||
| Span span = observationHelper.createSpan(AnalyticsEvents.GIT_SERIALIZE_APP_RESOURCES_TO_LOCAL_FILE.getEventName()); | ||
| observationHelper.startSpan(span, true); |
There was a problem hiding this comment.
Properly manage span activation and deactivation
Ensure that the span is activated in the current context to correctly capture trace information. Use try-with-resources for automatic scope management.
Apply this diff to improve span management:
Timer.Sample sample = Timer.start(meterRegistry);
Span span = observationHelper.createSpan(AnalyticsEvents.GIT_SERIALIZE_APP_RESOURCES_TO_LOCAL_FILE.getEventName());
-observationHelper.startSpan(span, true);
+try (Tracer.SpanInScope scope = tracer.withSpan(span)) {
// Existing code inside the try block
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (1)
5-5: Well-structured integration of Micrometer metrics.The addition of
MeterRegistryandObservationHelperaligns with enterprise observability patterns. This change effectively transitions from basic stopwatch logging to a more sophisticated metrics collection system.Consider documenting the key metrics being collected in the class-level JavaDoc to help other developers understand the observability touchpoints.
Also applies to: 13-13, 31-32, 40-41
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java(2 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (1)
24-32: LGTM! Clean constructor parameter organization.
The constructor parameters are well-organized, following the pattern of core dependencies first (git utils, file operations) followed by supporting services (analytics, session) and finally the new observability components.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Bug Issue
Description:
The stopwatch metrics are only being sent to logs, and we haven't been able to fully utilize them.
With Micrometer now handling our metrics collection, we can eliminate the stopwatch metrics in CommonGitFileUtils, as they no longer add value and are limited to log outputs.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
TimerandObservationHelperfor better performance monitoring.