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

chore: Add JSObject creation flow metrics #37064

Merged
merged 3 commits into from
Oct 28, 2024
Merged

chore: Add JSObject creation flow metrics #37064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ec91dd
chore: Add JSObject creation flow metrics
Oct 24, 2024
abe0d13
undo the attribute related changes
Oct 24, 2024
ebf826f
rename span to follow nomenclature
Oct 24, 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 ...smith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionSpanCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,9 @@ public class ActionSpanCE {
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";

public static final String VALIDATE_AND_GENERATE_ACTION_DOMAIN_BASED_ON_CONTEXT =
Copy link
Contributor

Choose a reason for hiding this comment

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

@rishabhrathod01 can we please follow the nomenclature for spans which uses the namespace of the logical product area that a span is added for? For example, consolidated spans are under appsmith.consolidated-api , git spans are under appsmith.git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nidhi-nair There was a concern about span name truncation after 50 chars on newRelic due to which it becomes difficult to filter them. Initially, I followed the practice of appsmith.domain.class.methodName or appsmith.domain.methodName for few span but that exceeds the char limit.

I will update the changes to follow the nomenclature by limiting the domain name char.

APPSMITH_SPAN_PREFIX + ACTIONS + "validateAndGenerateActionDomainBasedOnContext";
public static final String VALIDATE_AND_SAVE_ACTION_TO_REPOSITORY =
APPSMITH_SPAN_PREFIX + ACTIONS + "validateAndSaveActionToRepository";
}
5 changes: 5 additions & 0 deletions ...h-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/DatasourceSpanCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
import static com.appsmith.external.constants.spans.BaseSpan.APPSMITH_SPAN_PREFIX;

public class DatasourceSpanCE {
public static final String DATASOURCES = "datasources.";
public static final String FETCH_ALL_DATASOURCES_WITH_STORAGES =
APPSMITH_SPAN_PREFIX + "get_all_datasource_storage";
public static final String FETCH_ALL_PLUGINS_IN_WORKSPACE = APPSMITH_SPAN_PREFIX + "get_all_plugins_in_workspace";

// datasource service spans
public static final String DATASOURCE_SERVICE = "datasourceService";
public static final String FIND_DATASOURCE_BY_ID = APPSMITH_SPAN_PREFIX + DATASOURCE_SERVICE + ".findById";
Copy link
Contributor

Choose a reason for hiding this comment

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

@rishabhrathod01 Should we use domain name here instead of datasourceService? I see you have already declared the domain of datasources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sneha122 I intended to create datasourceService as a domain name here to ease finding the method.

}
7 changes: 5 additions & 2 deletions ...ppsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/PageSpanCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ public class PageSpanCE {
public static final String PREPARE_APPLICATION_PAGES_DTO_FROM_PAGES = PAGES + "generate_app_pages_dto";
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";
public static final String GET_PAGE_BY_ID = APPSMITH_SPAN_PREFIX + PAGES + "newPageService.findById";
public static final String GET_PAGE_BY_ID_AND_LAYOUTS_ID = APPSMITH_SPAN_PREFIX + PAGES + "getPageByIdAndLayoutsId";

// page level method added here
public static final String IS_NAME_ALLOWED = APPSMITH_SPAN_PREFIX + PAGES + "refactoringService.isNameAllowed";
}
22 changes: 19 additions & 3 deletions ...smith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,12 @@
import static com.appsmith.external.constants.spans.ActionSpan.GET_ACTION_BY_ID;
import static com.appsmith.external.constants.spans.ActionSpan.UPDATE_ACTION_BASED_ON_CONTEXT;
import static com.appsmith.external.constants.spans.ActionSpan.UPDATE_SINGLE_ACTION;
import static com.appsmith.external.constants.spans.ActionSpan.VALIDATE_AND_GENERATE_ACTION_DOMAIN_BASED_ON_CONTEXT;
import static com.appsmith.external.constants.spans.ActionSpan.VALIDATE_AND_SAVE_ACTION_TO_REPOSITORY;
import static com.appsmith.external.constants.spans.DatasourceSpan.FIND_DATASOURCE_BY_ID;
import static com.appsmith.external.constants.spans.LayoutSpan.UPDATE_PAGE_LAYOUT_BY_PAGE_ID;
import static com.appsmith.external.constants.spans.PageSpan.GET_PAGE_BY_ID;
import static com.appsmith.external.constants.spans.PageSpan.IS_NAME_ALLOWED;
import static java.util.stream.Collectors.toSet;

@Slf4j
Expand Down Expand Up @@ -337,6 +342,8 @@ protected Mono<ActionDTO> createAction(ActionDTO actionDTO, Boolean isJsAction)
public Mono<ActionDTO> createAction(ActionDTO actionDTO, AppsmithEventContext eventContext, Boolean isJsAction) {

return validateAndGenerateActionDomainBasedOnContext(actionDTO, isJsAction)
.name(VALIDATE_AND_GENERATE_ACTION_DOMAIN_BASED_ON_CONTEXT)
.tap(Micrometer.observation(observationRegistry))
.flatMap(newAction -> {
// If the datasource is embedded, check for workspaceId and set it in action
if (actionDTO.getDatasource() != null
Expand All @@ -361,13 +368,17 @@ public Mono<ActionDTO> createAction(ActionDTO actionDTO, AppsmithEventContext ev
})
.flatMap(savedNewAction -> newActionService
.validateAndSaveActionToRepository(savedNewAction)
.name(VALIDATE_AND_SAVE_ACTION_TO_REPOSITORY)
.tap(Micrometer.observation(observationRegistry))
.zipWith(Mono.just(savedNewAction)))
.zipWhen(zippedActions -> {
ActionDTO savedActionDTO = zippedActions.getT1();
if (savedActionDTO.getDatasource() != null
&& savedActionDTO.getDatasource().getId() != null) {
return datasourceService.findById(
savedActionDTO.getDatasource().getId());
return datasourceService
.findById(savedActionDTO.getDatasource().getId())
.name(FIND_DATASOURCE_BY_ID)
.tap(Micrometer.observation(observationRegistry));
} else {
return Mono.justOrEmpty(savedActionDTO.getDatasource());
}
Expand Down Expand Up @@ -398,6 +409,8 @@ protected Mono<NewAction> validateAndGenerateActionDomainBasedOnContext(ActionDT

Mono<NewPage> pageMono = newPageService
.findById(action.getPageId(), aclPermission)
.name(GET_PAGE_BY_ID)
.tap(Micrometer.observation(observationRegistry))
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, action.getPageId())))
.cache();
Expand All @@ -409,7 +422,10 @@ protected Mono<NewAction> validateAndGenerateActionDomainBasedOnContext(ActionDT
String name = action.getValidName();
CreatorContextType contextType =
action.getContextType() == null ? CreatorContextType.PAGE : action.getContextType();
return refactoringService.isNameAllowed(page.getId(), contextType, layout.getId(), name);
return refactoringService
.isNameAllowed(page.getId(), contextType, layout.getId(), name)
.name(IS_NAME_ALLOWED)
.tap(Micrometer.observation(observationRegistry));
})
.flatMap(nameAllowed -> {
// If the name is allowed, return pageMono for further processing
Expand Down
Loading