Skip to content

SED-4157-executions history #577

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

Open
lucianbaciu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SED-4157-executions history #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion step-controller/step-controller-backend/src/test/resources/step.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ plugins.selenium.libs.3.x=../../../../step-distribution/step-distribution-parent
plugins.selenium.libs.2.x=../../../../step-distribution/step-distribution-parent/step-distribution-controller/template-controller/ext/selenium/selenium-2.53.1
plugins.jmeter.home=../../../../step-distribution/step-distribution-parent/step-distribution-controller/template-controller/ext/jmeter
plugins.javascript.libs=../../../../step-distribution/step-distribution-parent/step-distribution-controller/template-controller/ext/javascript
plugins.groovy.libs=../../../../step-distribution/step-distribution-parent/step-distribution-controller/template-controller/ext/groovy
plugins.groovy.libs=../../../step-distribution/step-distribution-controller/template-controller/ext/groovy
authentication=true
# Defines the path to the embedded function packages
# plugins.FunctionPackagePlugin.embeddedpackages.folder=../../../../step-distribution/step-distribution-parent/step-distribution-controller/template-controller/plugins/keywords
Expand Down
9 changes: 9 additions & 0 deletions step-core/src/main/java/step/core/repositories/ImportResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public class ImportResult implements Serializable {
protected boolean successful = false;;

protected String planId;
protected String canonicalPlanName;

List<String> errors;

Expand All @@ -47,6 +48,14 @@ public void setPlanId(String planId) {
this.planId = planId;
}

public String getCanonicalPlanName() {
return canonicalPlanName;
}

public void setCanonicalPlanName(String canonicalPlanName) {
this.canonicalPlanName = canonicalPlanName;
}

public List<String> getErrors() {
return errors;
}
Expand Down
39 changes: 39 additions & 0 deletions .../step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package step.reporting;

import step.core.execution.ExecutionContext;
import step.core.execution.model.Execution;
import step.core.execution.model.ExecutionAccessor;
import step.core.execution.model.ExecutionResultSnapshot;
import step.core.plugins.Plugin;
import step.engine.plugins.AbstractExecutionEnginePlugin;

import java.util.List;
import java.util.stream.Collectors;

@Plugin
public class ExecutionHistoryReportPlugin extends AbstractExecutionEnginePlugin {

@Override
public void executionStart(ExecutionContext context) {
// TOOD collect the executions here too
Copy link

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

medium

There is a typo in the comment. It should be TODO.

Suggested change
// TOOD collect the executions here too
// TODO collect the executions here too

}

@Override
public void afterExecutionEnd(ExecutionContext context) {
ExecutionAccessor executionAccessor = context.getExecutionAccessor();
Execution execution = context.getExecutionManager().getExecution();
// endTime is not set here
long searchBeforeTimestamp = System.currentTimeMillis() - 1;
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

This might be true now, but can change in the future, you should first check the execution end time and only if not set set use the current time.

List<ExecutionResultSnapshot> pastExecutionsSnapshots = executionAccessor.getLastEndedExecutionsByCanonicalPlanName(execution.getCanonicalPlanName(), 10, searchBeforeTimestamp)
Copy link

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

medium

The number of past executions to fetch is hardcoded to 10. This value should be made configurable to allow for flexibility in different environments. I recommend extracting it into a constant, and ideally, loading it from a configuration file or system property.

Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

agreed, get the value from the configuration (step.properties) with a default value of 10.

.stream()
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

getLastEndedExecutionsByCanonicalPlanName should return a stream directly

.map(e -> new ExecutionResultSnapshot()
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

Think of the information you'll need to also display a tooltip, at least the execution start time would be required too

.setId(e.getId().toString())
.setResult(e.getResult())
.setStatus(e.getStatus())
)
.collect(Collectors.toList());
execution.setHistoryResults(pastExecutionsSnapshots);
executionAccessor.save(execution);
}

}
25 changes: 24 additions & 1 deletion step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,27 @@ protected PlanRunnerResult execute() {
addImportResultToExecution(importResult);
saveFailureReportWithResult(ReportNodeStatus.VETOED);
} else {
String canonicalPlanName;
try {
Plan plan = getPlanFromExecutionParametersOrImport();
ExecutionParameters executionParameters = executionContext.getExecutionParameters();
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

So as discussed please revert to calling the method getPlanFromExecutionParametersOrImport, and imply update the execution canonical name from within that method. We already persist the import result their.

Plan plan = executionParameters.getPlan();
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

You could simply check where we use the executionParameters.setPlan to see that this case is used for interactive execution (see startInteractiveSession in InteractiveServices), if you want more details or debug the case. This case is edgy and it would actually be cleaner and more correct to not create any historical statuses for them (or even ingest any time-series for them). For now I would then not set the canonical name and handle null value in the plugin

if (plan == null) {
ImportResult importResult = importPlan(executionContext);
canonicalPlanName = importResult.getCanonicalPlanName();
addImportResultToExecution(importResult);
if (importResult.isSuccessful()) {
PlanAccessor planAccessor = executionContext.getPlanAccessor();
plan = planAccessor.get(new ObjectId(importResult.getPlanId()));
} else {
throw new PlanImportException();
}
} else {
// plan already exists in memory
canonicalPlanName = plan.getId().toString();
Comment on lines +104 to +105
Copy link

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

high

When a plan is passed directly in ExecutionParameters, the canonical plan name is set to the plan's ID. This can lead to inconsistencies if the same plan is sometimes imported (getting a canonical name from repository parameters) and sometimes passed directly. To ensure consistency, you should try to generate the canonical name from executionParameters.getRepositoryObject() if it's available, even when the plan object is already present. The current approach might result in fragmented execution histories for the same logical plan.

This will require adding import static step.core.repositories.AbstractRepository.getCanonicalPlanName; to the file.

						// plan already exists in memory
						RepositoryObjectReference repositoryObject = executionParameters.getRepositoryObject();
						if (repositoryObject != null) {
							canonicalPlanName = getCanonicalPlanName(repositoryObject.getRepositoryParameters());
						} else {
							canonicalPlanName = plan.getId().toString();
						}

}

addPlanToContextAndUpdateExecution(plan);
addCanonicalPlanNameToExecution(canonicalPlanName);

logger.info(messageWithId("Starting execution."));
updateStatus(ExecutionStatus.ESTIMATING);
Expand Down Expand Up @@ -176,6 +194,7 @@ private List<ExecutionVeto> getExecutionVetoes() {
.collect(Collectors.toList());
}

// TODO from here we need to extract the importResult, somehow
private Plan getPlanFromExecutionParametersOrImport() throws PlanImportException {
ExecutionParameters executionParameters = executionContext.getExecutionParameters();
Plan executionParametersPlan = executionParameters.getPlan();
Expand Down Expand Up @@ -209,6 +228,10 @@ private void addPlanToContextAndUpdateExecution(Plan plan) {
});
}

private void addCanonicalPlanNameToExecution(String canonicalPlanName) {
updateExecution(execution -> execution.setCanonicalPlanName(canonicalPlanName));
}

private String messageWithId(String message) {
return message + " Execution ID: " + executionContext.getExecutionId();
}
Expand Down
19 changes: 19 additions & 0 deletions step-plans/step-plans-core/src/main/java/step/core/execution/model/Execution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public class Execution extends AbstractOrganizableObject implements EnricheableO
private ReportNodeStatus result;
private List<Error> lifecycleErrors;
private String planId;
private String canonicalPlanName;
private ImportResult importResult;
private List<ReportExport> reportExports;
private String executionTaskID;
Expand All @@ -54,6 +55,7 @@ public class Execution extends AbstractOrganizableObject implements EnricheableO
private ExecutiontTaskParameters executiontTaskParameters;
private String resolvedPlanRootNodeId;
private String agentsInvolved;
private List<ExecutionResultSnapshot> historyResults;

public Execution() {
super();
Expand Down Expand Up @@ -148,6 +150,14 @@ public void setPlanId(String planId) {
this.planId = planId;
}

public String getCanonicalPlanName() {
return canonicalPlanName;
}

public void setCanonicalPlanName(String canonicalPlanName) {
this.canonicalPlanName = canonicalPlanName;
}

/**
* @return the result of the import phase from the external repository (ALM, Jira, etc)
*/
Expand Down Expand Up @@ -245,6 +255,15 @@ public void setAgentsInvolved(String agentsInvolved) {
this.agentsInvolved = agentsInvolved;
}

public List<ExecutionResultSnapshot> getHistoryResults() {
return historyResults;
}

public Execution setHistoryResults(List<ExecutionResultSnapshot> historyResults) {
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

Use at standard setter, must be void

this.historyResults = historyResults;
return this;
}

@Override
public String toString() {
return "Execution [startTime=" + startTime + ", endTime=" + endTime + ", description=" + description
Expand Down
1 change: 1 addition & 0 deletions step-plans/step-plans-core/src/main/java/step/core/execution/model/ExecutionAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,5 @@ public interface ExecutionAccessor extends Accessor<Execution>, ExecutionProvide

List<Execution> getLastEndedExecutionsBySchedulerTaskID(String schedulerTaskID, int limit, Long from, Long to);

List<Execution> getLastEndedExecutionsByCanonicalPlanName(String canonicalPlanName, int limit, Long from);
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

Should return the stream directly

}
20 changes: 20 additions & 0 deletions ...-plans/step-plans-core/src/main/java/step/core/execution/model/ExecutionAccessorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public void createIndexesIfNeeded(Long ttl) {
new IndexField("endTime",Order.DESC, null))));
collectionDriver.createOrUpdateCompoundIndex(new LinkedHashSet<>(List.of(new IndexField("planId",Order.ASC, null),
new IndexField("endTime",Order.DESC, null))));
collectionDriver.createOrUpdateCompoundIndex(new LinkedHashSet<>(List.of(new IndexField("canonicalPlanName",Order.ASC, null),
new IndexField("endTime",Order.DESC, null))));
}

@Override
Expand Down Expand Up @@ -174,4 +176,22 @@ public List<Execution> getLastEndedExecutionsBySchedulerTaskID(String schedulerT
order, 0, limit, 0)
.collect(Collectors.toList());
}

@Override
public List<Execution> getLastEndedExecutionsByCanonicalPlanName(String canonicalPlanName, int limit, Long from) {
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

The provided timestamp is used to filter execution ending before it, please rename accordingly

SearchOrder order = new SearchOrder("endTime", -1);

List<Filter> filters = new ArrayList<>(List.of(
Filters.equals("canonicalPlanName", canonicalPlanName),
Filters.equals("status", ExecutionStatus.ENDED.name())
));

if (from != null) {
filters.add(Filters.lte("endTime", from));
}

return collectionDriver
.find(Filters.and(filters), order, 0, limit, 0)
.collect(Collectors.toList());
}
}
37 changes: 37 additions & 0 deletions ...lans/step-plans-core/src/main/java/step/core/execution/model/ExecutionResultSnapshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package step.core.execution.model;

import step.core.artefacts.reports.ReportNodeStatus;

public class ExecutionResultSnapshot {

private String id;
private ExecutionStatus status;
private ReportNodeStatus result;

public String getId() {
return id;
}

public ExecutionResultSnapshot setId(String id) {
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

Use standard setters (void) for POJO, this is not a builder.

this.id = id;
return this;
}

public ExecutionStatus getStatus() {
return status;
}

public ExecutionResultSnapshot setStatus(ExecutionStatus status) {
this.status = status;
return this;
}

public ReportNodeStatus getResult() {
return result;
}

public ExecutionResultSnapshot setResult(ReportNodeStatus result) {
this.result = result;
return this;
}
}
12 changes: 12 additions & 0 deletions step-plans/step-plans-core/src/main/java/step/core/repositories/AbstractRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@ private static Map<String, String> getCanonicalRepositoryParameters(Set<String>
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)) : null;
}

public static String getCanonicalPlanName(Map<String, String> repositoryParameters) {
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

This cannot be a static method since you need access to the canonical keys set

if (repositoryParameters == null || repositoryParameters.isEmpty()) {
return "";
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

We should return null to be consistent and let the caller know that no canonical name exist

}

return repositoryParameters.entrySet().stream()
.sorted(Map.Entry.comparingByKey())
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

You are not filtering by canonicalRepositoryParameters, this is not a canonical representation.

.map(e -> e.getKey() + "=" + e.getValue())
.collect(java.util.stream.Collectors.joining("&"));
Copy link

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

medium

The fully qualified name java.util.stream.Collectors is used here, but java.util.stream.Collectors is already imported. You can simplify this by using just Collectors.joining("&").

Suggested change
.collect(java.util.stream.Collectors.joining("&"));
.collect(Collectors.joining("&"));

}


@Override
public Set<String> getCanonicalRepositoryParameters() {
return canonicalRepositoryParameters;
Expand Down
1 change: 1 addition & 0 deletions step-plans/step-plans-core/src/main/java/step/core/repositories/Repository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,5 @@ default void postExecution(ExecutionContext context, RepositoryObjectReference r
boolean compareCanonicalRepositoryParameters(Map<String, String> repositoryParameters1, Map<String, String> repositoryParameters2);

Set<String> getCanonicalRepositoryParameters();

}
6 changes: 5 additions & 1 deletion step-plans/step-plans-core/src/main/java/step/core/repositories/RepositoryObjectManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

import static step.core.repositories.AbstractRepository.getCanonicalPlanName;

public class RepositoryObjectManager {

private static final Logger logger = LoggerFactory.getLogger(RepositoryObjectManager.class);
Expand All @@ -48,7 +50,9 @@ public void registerRepository(String id, Repository repository) {
public ImportResult importPlan(ExecutionContext context, RepositoryObjectReference artefact) throws Exception {
String respositoryId = artefact.getRepositoryID();
Repository repository = getRepository(respositoryId);
return repository.importArtefact(context, artefact.getRepositoryParameters());
ImportResult importResult = repository.importArtefact(context, artefact.getRepositoryParameters());
importResult.setCanonicalPlanName(getCanonicalPlanName(artefact.getRepositoryParameters()));
Copy link
Contributor

@david-stephan david-stephan Dec 19, 2025

Choose a reason for hiding this comment

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

Cannot be a static call, you have the repository instance right above.

return importResult;
}

public Repository getRepository(String respositoryId) {
Expand Down