Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,6 @@ public class ActionSpanCE {
public static final String CREATE_ACTION = APPSMITH_SPAN_PREFIX + "create.action";
public static final String UPDATE_ACTION = APPSMITH_SPAN_PREFIX + "update.action";
public static final String DELETE_ACTION = APPSMITH_SPAN_PREFIX + "delete.action";
public static final String FILL_SELF_REFERENCING_PATHS_ACTION =
APPSMITH_SPAN_PREFIX + "action.fillSelfReferencingPaths";
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ public class ApplicationSpanCE {
public static final String APPLICATION_FETCH_FROM_DB = CONSOLIDATED_API_PREFIX + "app_db";
public static final String APPLICATION_ID_FETCH_REDIS_SPAN = APPLICATION_ID_SPAN + ".read_redis";
public static final String APPLICATION_ID_UPDATE_REDIS_SPAN = APPLICATION_ID_SPAN + ".update_redis";
public static final String APPLICATION_SAVE_LAST_EDIT_INFO_SPAN = APPLICATION_ID_SPAN + ".save_last_edit_info";
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,14 @@ public class LayoutSpanCE {
APPSMITH_SPAN_PREFIX + "onLoadExecutablesUtil.updateExecutablesExecuteOnLoad";
public static final String FIND_AND_UPDATE_LAYOUT =
APPSMITH_SPAN_PREFIX + "onLoadExecutablesUtil.findAndUpdateLayout";

public static final String EXTRACT_AND_SET_EXECUTABLE_BINDINGS_IN_GRAPH_EDGES =
APPSMITH_SPAN_PREFIX + "extractAndSetExecutableBindingsInGraphEdges";
public static final String RECURSIVELY_ADD_EXECUTABLES_AND_THEIR_DEPENDENTS_TO_GRAPH_FROM_BINDINGS =
APPSMITH_SPAN_PREFIX + "recursivelyAddExecutablesAndTheirDependentsToGraphFromBindings";
public static final String ADD_WIDGET_RELATIONSHIP_TO_GRAPH = APPSMITH_SPAN_PREFIX + "addWidgetRelationshipToGraph";
public static final String COMPUTE_ON_PAGE_LOAD_EXECUTABLES_SCHEDULING_ORDER =
APPSMITH_SPAN_PREFIX + "computeOnPageLoadExecutablesSchedulingOrder";
public static final String FILTER_AND_TRANSFORM_SCHEDULING_ORDER_TO_DTO =
APPSMITH_SPAN_PREFIX + "filterAndTransformSchedulingOrderToDTO";
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ public class PageSpanCE {
public static final String MIGRATE_DSL = PAGES + "migrate_dsl";

public static final String GET_PAGE_BY_ID = APPSMITH_SPAN_PREFIX + "get.page.unpublished";
public static final String GET_PAGE_BY_ID_AND_LAYOUTS_ID = APPSMITH_SPAN_PREFIX + "getPageByIdAndLayoutsId";
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,21 @@
import com.appsmith.server.onload.executables.ExecutableOnLoadServiceCE;
import com.appsmith.server.solutions.ActionPermission;
import com.appsmith.server.solutions.PagePermission;
import io.micrometer.observation.ObservationRegistry;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.BeanUtils;
import org.springframework.stereotype.Service;
import reactor.core.observability.micrometer.Micrometer;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;

import java.util.List;

import static com.appsmith.external.constants.spans.ActionSpan.FILL_SELF_REFERENCING_PATHS_ACTION;
import static com.appsmith.external.constants.spans.ApplicationSpan.APPLICATION_SAVE_LAST_EDIT_INFO_SPAN;
import static com.appsmith.external.constants.spans.PageSpan.GET_PAGE_BY_ID_AND_LAYOUTS_ID;

@Slf4j
@RequiredArgsConstructor
@Service
Expand All @@ -34,6 +40,7 @@ public class ExecutableOnPageLoadServiceCEImpl implements ExecutableOnLoadServic

private final ActionPermission actionPermission;
private final PagePermission pagePermission;
private final ObservationRegistry observationRegistry;

@Override
public Flux<Executable> getAllExecutablesByCreatorIdFlux(String creatorId) {
Expand All @@ -48,6 +55,8 @@ public Flux<Executable> getAllExecutablesByCreatorIdFlux(String creatorId) {
public Mono<Executable> fillSelfReferencingPaths(Executable executable) {
return newActionService
.fillSelfReferencingDataPaths((ActionDTO) executable)
.name(FILL_SELF_REFERENCING_PATHS_ACTION)
.tap(Micrometer.observation(observationRegistry))
.map(actionDTO -> actionDTO);
}

Expand All @@ -69,6 +78,8 @@ public Mono<Executable> updateUnpublishedExecutable(String id, Executable execut
public Mono<Layout> findAndUpdateLayout(String creatorId, String layoutId, Layout layout) {
Mono<PageDTO> pageDTOMono = newPageService
.findByIdAndLayoutsId(creatorId, layoutId, pagePermission.getEditPermission(), false)
.name(GET_PAGE_BY_ID_AND_LAYOUTS_ID)
.tap(Micrometer.observation(observationRegistry))
.switchIfEmpty(Mono.error(new AppsmithException(
AppsmithError.ACL_NO_RESOURCE_FOUND,
FieldName.PAGE_ID + " or " + FieldName.LAYOUT_ID,
Expand All @@ -93,6 +104,8 @@ public Mono<Layout> findAndUpdateLayout(String creatorId, String layoutId, Layou
page.setLayouts(layoutList);
return applicationService
.saveLastEditInformation(page.getApplicationId())
.name(APPLICATION_SAVE_LAST_EDIT_INFO_SPAN)
.tap(Micrometer.observation(observationRegistry))
.then(newPageService.saveUnpublishedPage(page));
})
.flatMap(page -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.appsmith.server.onload.executables.ExecutableOnLoadService;
import com.appsmith.server.solutions.ActionPermission;
import com.appsmith.server.solutions.PagePermission;
import io.micrometer.observation.ObservationRegistry;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service;

Expand All @@ -19,7 +20,14 @@ public ExecutableOnPageLoadServiceImpl(
NewPageService newPageService,
ApplicationService applicationService,
ActionPermission actionPermission,
PagePermission pagePermission) {
super(newActionService, newPageService, applicationService, actionPermission, pagePermission);
PagePermission pagePermission,
ObservationRegistry observationRegistry) {
super(
newActionService,
newPageService,
applicationService,
actionPermission,
pagePermission,
observationRegistry);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,20 @@
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.ObservationHelperImpl;
import com.appsmith.server.onload.executables.ExecutableOnLoadService;
import com.appsmith.server.services.AstService;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.micrometer.observation.ObservationRegistry;
import io.micrometer.tracing.Span;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import net.minidev.json.JSONObject;
import org.jgrapht.graph.DefaultEdge;
import org.jgrapht.graph.DirectedAcyclicGraph;
import org.jgrapht.traverse.BreadthFirstIterator;
import reactor.core.observability.micrometer.Micrometer;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.util.function.Tuple2;
Expand All @@ -43,6 +47,11 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import static com.appsmith.external.constants.spans.LayoutSpan.ADD_WIDGET_RELATIONSHIP_TO_GRAPH;
import static com.appsmith.external.constants.spans.LayoutSpan.COMPUTE_ON_PAGE_LOAD_EXECUTABLES_SCHEDULING_ORDER;
import static com.appsmith.external.constants.spans.LayoutSpan.EXTRACT_AND_SET_EXECUTABLE_BINDINGS_IN_GRAPH_EDGES;
import static com.appsmith.external.constants.spans.LayoutSpan.FILTER_AND_TRANSFORM_SCHEDULING_ORDER_TO_DTO;
import static com.appsmith.external.constants.spans.LayoutSpan.RECURSIVELY_ADD_EXECUTABLES_AND_THEIR_DEPENDENTS_TO_GRAPH_FROM_BINDINGS;
import static com.appsmith.external.helpers.MustacheHelper.EXECUTABLE_ENTITY_REFERENCES;
import static com.appsmith.external.helpers.MustacheHelper.WIDGET_ENTITY_REFERENCES;
import static com.appsmith.external.helpers.MustacheHelper.getPossibleParents;
Expand Down Expand Up @@ -71,6 +80,8 @@ public class OnLoadExecutablesUtilCEImpl implements OnLoadExecutablesUtilCE {
// TODO : This should contain all the static global variables present on the page like `appsmith`, etc.
// TODO : Add all the global variables exposed on the client side.
private final Set<String> APPSMITH_GLOBAL_VARIABLES = Set.of();
private final ObservationRegistry observationRegistry;
private final ObservationHelperImpl observationHelper;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use interface type for observationHelper

Consider declaring observationHelper as ObservationHelper instead of ObservationHelperImpl to depend on the interface rather than the implementation.

Apply this diff:

-private final ObservationHelperImpl observationHelper;
+private final ObservationHelper observationHelper;
📝 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
private final ObservationHelperImpl observationHelper;
private final ObservationHelper observationHelper;


/**
* This function computes the sequenced on page load executables.
Expand Down Expand Up @@ -174,11 +185,15 @@ public Mono<List<Set<DslExecutableDTO>>> findAllOnLoadExecutables(
bindingsFromExecutablesRef,
executableNameToExecutableMapMono,
evaluatedVersion))
.name(RECURSIVELY_ADD_EXECUTABLES_AND_THEIR_DEPENDENTS_TO_GRAPH_FROM_BINDINGS)
.tap(Micrometer.observation(observationRegistry))
Comment on lines +188 to +189
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor repetitive Micrometer instrumentation code

The code pattern with .name(...) and .tap(Micrometer.observation(observationRegistry)) is used multiple times. Consider creating a utility method to encapsulate this pattern and reduce duplication.

Example:

private <T> Mono<T> instrument(Mono<T> mono, String name) {
    return mono
        .name(name)
        .tap(Micrometer.observation(observationRegistry));
}

Then, replace:

someMonoOperation()
    .name(SOME_CONSTANT)
    .tap(Micrometer.observation(observationRegistry));

with:

instrument(someMonoOperation(), SOME_CONSTANT);

Also applies to: 649-650, 930-931, 948-949, 1002-1003

// At last, add all the widget relationships to the graph as well.
.zipWith(executablesInCreatorContextMono)
.flatMap(tuple -> {
Set<ExecutableDependencyEdge> updatedEdges = tuple.getT1();
return addWidgetRelationshipToGraph(updatedEdges, widgetDynamicBindingsMap, evaluatedVersion);
return addWidgetRelationshipToGraph(updatedEdges, widgetDynamicBindingsMap, evaluatedVersion)
.name(ADD_WIDGET_RELATIONSHIP_TO_GRAPH)
.tap(Micrometer.observation(observationRegistry));
});

// Create a graph given edges
Expand All @@ -198,11 +213,20 @@ public Mono<List<Set<DslExecutableDTO>>> findAllOnLoadExecutables(
Map<String, Executable> executableNameToExecutableMap = tuple.getT1();
DirectedAcyclicGraph<String, DefaultEdge> graph = tuple.getT2();

return computeOnPageLoadExecutablesSchedulingOrder(
Span computeOnPageLoadExecutablesSchedulingOrderSpan =
observationHelper.createSpan(COMPUTE_ON_PAGE_LOAD_EXECUTABLES_SCHEDULING_ORDER);

observationHelper.startSpan(computeOnPageLoadExecutablesSchedulingOrderSpan, true);

List<Set<String>> executablesList = computeOnPageLoadExecutablesSchedulingOrder(
graph,
onLoadExecutableSetRef,
executableNameToExecutableMap,
explicitUserSetOnLoadExecutablesRef);

observationHelper.endSpan(computeOnPageLoadExecutablesSchedulingOrderSpan, true);

return executablesList;
})
.map(onPageLoadExecutablesSchedulingOrder -> {
// Find all explicitly turned on executables which haven't found their way into the scheduling order
Expand Down Expand Up @@ -237,6 +261,8 @@ public Mono<List<Set<DslExecutableDTO>>> findAllOnLoadExecutables(
onLoadExecutableSetRef,
executableNameToExecutableMapMono,
computeOnPageLoadScheduleNamesMono)
.name(FILTER_AND_TRANSFORM_SCHEDULING_ORDER_TO_DTO)
.tap(Micrometer.observation(observationRegistry))
.cache();

// With the final on page load scheduling order, also set the on page load executables which would be updated
Expand Down Expand Up @@ -620,6 +646,8 @@ private Mono<Set<ExecutableDependencyEdge>> addDirectlyReferencedExecutablesToGr
executablesFoundDuringWalkRef,
null,
evalVersion))
.name(EXTRACT_AND_SET_EXECUTABLE_BINDINGS_IN_GRAPH_EDGES)
.tap(Micrometer.observation(observationRegistry))
.thenReturn(possibleEntity);
}
return Mono.just(possibleEntity);
Expand Down Expand Up @@ -899,6 +927,8 @@ private Mono<Set<ExecutableDependencyEdge>> recursivelyAddExecutablesAndTheirDep
executablesFoundDuringWalk,
null,
evalVersion))
.name(EXTRACT_AND_SET_EXECUTABLE_BINDINGS_IN_GRAPH_EDGES)
.tap(Micrometer.observation(observationRegistry))
.thenReturn(possibleEntity);
} else {
return Mono.empty();
Expand All @@ -910,7 +940,13 @@ private Mono<Set<ExecutableDependencyEdge>> recursivelyAddExecutablesAndTheirDep
// Now that the next set of bindings have been found, find recursively all executables by these names
// and their bindings
return recursivelyAddExecutablesAndTheirDependentsToGraphFromBindings(
edges, executablesFoundDuringWalk, newBindings, executableNameToExecutableMapMono, evalVersion);
edges,
executablesFoundDuringWalk,
newBindings,
executableNameToExecutableMapMono,
evalVersion)
.name(RECURSIVELY_ADD_EXECUTABLES_AND_THEIR_DEPENDENTS_TO_GRAPH_FROM_BINDINGS)
.tap(Micrometer.observation(observationRegistry));
});
}

Expand Down Expand Up @@ -963,6 +999,8 @@ private Mono<Set<ExecutableDependencyEdge>> addExplicitUserSetOnLoadExecutablesT
executablesFoundDuringWalkRef,
executableBindingsInDsl,
evalVersion)
.name(EXTRACT_AND_SET_EXECUTABLE_BINDINGS_IN_GRAPH_EDGES)
.tap(Micrometer.observation(observationRegistry))
.thenReturn(executable);
})
.collectList()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package com.appsmith.server.onload.internal;

import com.appsmith.server.domains.NewPage;
import com.appsmith.server.helpers.ObservationHelperImpl;
import com.appsmith.server.onload.executables.ExecutableOnLoadService;
import com.appsmith.server.services.AstService;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.micrometer.observation.ObservationRegistry;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;

Expand All @@ -14,7 +16,9 @@ public class OnLoadExecutablesUtilImpl extends OnLoadExecutablesUtilCEImpl imple
public OnLoadExecutablesUtilImpl(
AstService astService,
ObjectMapper objectMapper,
ExecutableOnLoadService<NewPage> pageExecutableOnLoadService) {
super(astService, objectMapper, pageExecutableOnLoadService);
ExecutableOnLoadService<NewPage> pageExecutableOnLoadService,
ObservationRegistry observationRegistry,
ObservationHelperImpl observationHelper) {
super(astService, objectMapper, pageExecutableOnLoadService, observationRegistry, observationHelper);
}
}