Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: replaced the stopwatch implementation with micrometer #37200

Closed
wants to merge 2 commits into from
Closed

fix: replaced the stopwatch implementation with micrometer #37200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c3b9fb
fix: replaced the stopwatch implementation with micrometer
Sai6347 Nov 4, 2024
6d1e72d
fix:updated the missing argmuents in constructor
Sai6347 Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/server/appsmith-git/src/main/java/com/appsmith/git/constants/CommonConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,9 @@ public class CommonConstants {

public static final String WIDGET_ID = "widgetId";
public static final String PARENT_ID = "parentId";

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";
}
22 changes: 14 additions & 8 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

import com.appsmith.external.git.FileInterface;
import com.appsmith.external.git.operations.FileOperations;
import com.appsmith.external.helpers.ObservationHelper;
import com.appsmith.external.models.ApplicationGitReference;
import com.appsmith.git.files.FileUtilsImpl;
import com.appsmith.server.helpers.ce.CommonGitFileUtilsCE;
import com.appsmith.server.migrations.JsonSchemaVersions;
import com.appsmith.server.services.AnalyticsService;
import com.appsmith.server.services.SessionUserService;
import com.google.gson.Gson;
import io.micrometer.core.instrument.MeterRegistry;
import lombok.extern.slf4j.Slf4j;
import org.springframework.context.annotation.Import;
import org.springframework.stereotype.Component;
Expand All @@ -19,19 +21,23 @@
public class CommonGitFileUtils extends CommonGitFileUtilsCE {

public CommonGitFileUtils(
ArtifactGitFileUtils<ApplicationGitReference> applicationGitFileUtils,
FileInterface fileUtils,
FileOperations fileOperations,
AnalyticsService analyticsService,
SessionUserService sessionUserService,
Gson gson,
JsonSchemaVersions jsonSchemaVersions) {
ArtifactGitFileUtils<ApplicationGitReference> applicationGitFileUtils,
FileInterface fileUtils,
FileOperations fileOperations,
AnalyticsService analyticsService,
SessionUserService sessionUserService,
Gson gson,
JsonSchemaVersions jsonSchemaVersions,
MeterRegistry meterRegistry,
ObservationHelper observationHelper) {
super(
applicationGitFileUtils,
fileUtils,
fileOperations,
analyticsService,
sessionUserService,
jsonSchemaVersions);
jsonSchemaVersions,
meterRegistry,
observationHelper);
}
}
40 changes: 27 additions & 13 deletions ...er/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import com.appsmith.external.constants.AnalyticsEvents;
import com.appsmith.external.git.FileInterface;
import com.appsmith.external.git.operations.FileOperations;
import com.appsmith.external.helpers.Stopwatch;
import com.appsmith.external.helpers.ObservationHelper;
import com.appsmith.external.models.ApplicationGitReference;
import com.appsmith.external.models.ArtifactGitReference;
import com.appsmith.external.models.BaseDomain;
Expand All @@ -25,6 +25,9 @@
import com.google.gson.Gson;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Timer;
import io.micrometer.tracing.Span;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.eclipse.jgit.api.errors.GitAPIException;
Expand Down Expand Up @@ -66,6 +69,8 @@ public class CommonGitFileUtilsCE {
public final int INDEX_LOCK_FILE_STALE_TIME = 300;

private final JsonSchemaVersions jsonSchemaVersions;
private final MeterRegistry meterRegistry;
private final ObservationHelper observationHelper;

private ArtifactGitFileUtils<?> getArtifactBasedFileHelper(ArtifactType artifactType) {
if (ArtifactType.APPLICATION.equals(artifactType)) {
Expand Down Expand Up @@ -112,26 +117,28 @@ public Mono<Path> saveArtifactToLocalRepoWithAnalytics(
3. Save artifact to git repo
*/
// TODO: see if event needs to be generalised or kept specific
Stopwatch stopwatch = new Stopwatch(AnalyticsEvents.GIT_SERIALIZE_APP_RESOURCES_TO_LOCAL_FILE.getEventName());
Timer.Sample sample = Timer.start(meterRegistry);
Span span = observationHelper.createSpan(AnalyticsEvents.GIT_SERIALIZE_APP_RESOURCES_TO_LOCAL_FILE.getEventName());
observationHelper.startSpan(span, true);
Comment on lines +120 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

ArtifactGitFileUtils<?> artifactGitFileUtils =
getArtifactBasedFileHelper(artifactExchangeJson.getArtifactJsonType());
String artifactConstant = artifactGitFileUtils.getConstantsMap().get(FieldName.ARTIFACT_CONTEXT);

try {
Mono<Path> repoPathMono = saveArtifactToLocalRepo(baseRepoSuffix, artifactExchangeJson, branchName);
return Mono.zip(repoPathMono, sessionUserService.getCurrentUser()).flatMap(tuple -> {
stopwatch.stopTimer();
sample.stop(Timer.builder(CommonConstants.GIT_SAVE_ARTIFACT)
.description(CommonConstants.TIME_TAKEN_TO_SAVE_ARTIFACT)
.register(meterRegistry));
observationHelper.endSpan(span, true);
Path repoPath = tuple.getT1();
// Path to repo will be : ./container-volumes/git-repo/workspaceId/defaultApplicationId/repoName/
final Map<String, Object> data = Map.of(
artifactConstant,
repoPath.getParent().getFileName().toString(),
FieldName.ORGANIZATION_ID,
repoPath.getParent().getParent().getFileName().toString(),
FieldName.FLOW_NAME,
stopwatch.getFlow(),
"executionTime",
stopwatch.getExecutionTime());
"executionTime", sample.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

return analyticsService
.sendEvent(
AnalyticsEvents.UNIT_EXECUTION_TIME.getEventName(),
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

throw Exceptions.propagate(e);
}
}
Expand Down Expand Up @@ -202,30 +210,36 @@ private void setDatasourcesInArtifactReference(
public Mono<ArtifactExchangeJson> reconstructArtifactExchangeJsonFromGitRepoWithAnalytics(
String workspaceId, String baseArtifactId, String repoName, String branchName, ArtifactType artifactType) {

Stopwatch stopwatch = new Stopwatch(AnalyticsEvents.GIT_DESERIALIZE_APP_RESOURCES_FROM_FILE.getEventName());
Timer.Sample sample = Timer.start(meterRegistry);
Span span = observationHelper.createSpan(AnalyticsEvents.GIT_DESERIALIZE_APP_RESOURCES_FROM_FILE.getEventName());
observationHelper.startSpan(span, true);
Comment on lines +213 to +215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

ArtifactGitFileUtils<?> artifactGitFileUtils = getArtifactBasedFileHelper(artifactType);
Map<String, String> constantsMap = artifactGitFileUtils.getConstantsMap();
return Mono.zip(
reconstructArtifactExchangeJsonFromGitRepo(
workspaceId, baseArtifactId, repoName, branchName, artifactType),
sessionUserService.getCurrentUser())
.flatMap(tuple -> {
stopwatch.stopTimer();
sample.stop(Timer.builder(CommonConstants.GIT_DESERIALIZE_ARTIFACT)
.description(CommonConstants.TIME_TAKEN_TO_DESERIALIZE_ARTIFACT)
.register(meterRegistry));
observationHelper.endSpan(span, true);
final Map<String, Object> data = Map.of(
constantsMap.get(FieldName.ID),
baseArtifactId,
FieldName.ORGANIZATION_ID,
workspaceId,
FieldName.FLOW_NAME,
stopwatch.getFlow(),
"executionTime",
stopwatch.getExecutionTime());
"executionTime", sample.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

return analyticsService
.sendEvent(
AnalyticsEvents.UNIT_EXECUTION_TIME.getEventName(),
tuple.getT2().getUsername(),
data)
.thenReturn(tuple.getT1());
})
.doOnError(e -> {
observationHelper.endSpan(span, false);
log.error("Error deserializing artifact : {}", e.getMessage());
Comment on lines +239 to +242
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
})
.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);
});

});
}

Expand Down
Loading