FLPATH-377: workflow log #368

RichardW98 · 2023-05-23T20:07:37Z

What this PR does / why we need it:
display custom log in ui
Which issue(s) this PR fixes
Fixes #FLPATH-377

Change type

Impacted services

Workflow Service
Notification Service

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.

openshift-ci-robot · 2023-05-23T20:08:29Z

pkliczewski · 2023-05-24T07:13:32Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/enums/LogType.java

+
+public enum LogType {
+
+	TEXT(""), INFO("\u001B[32m"), WARNING("\u001B[33m"), ERROR("\u001B[34m"), END("\u001B[39m");


It seems to me that we are mixing few things here. We have log levels (info, warn, err) and not log levels text and end. I think we should split

pkliczewski · 2023-05-24T07:32:17Z

...ice/src/main/java/com/redhat/parodos/workflow/execution/entity/WorkFlowTaskExecutionLog.java

+	private String log;
+
+	public void addLog(String nextLog) {
+		log = log + "\n" + nextLog;


Why do we want to keep logs in single line?

I will remove \n

pkliczewski · 2023-05-24T07:33:46Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/log/WorkFlowTaskLog.java

+
+	@Override
+	public String toString() {
+		return logType.getCode() + logText + (logType.getCode().isEmpty() ? "" : LogType.END.getCode());


do we care about timestamp? How do we know what was the order of logs?

the current order is the sequence of calling addLog(), which will append the new log to end. but yes we need to support timestamp

eloycoto · 2023-05-24T10:18:23Z

I do not really like the implementation TBH. As a user experience, I think that it's not great. For me, the implementation should be something.

this.log.error()
this.log.info()

And execute context should be inherent inside the class, and the log should log the workflow.ID as wid='context.get(workflowid)' In this way, a user can retrieve all the logs related to a workflow based on a simple query in any 3-party tool (Loki, ElasticSearch, etc..)

Dancing the context with multiple functions is not the best, and a function like addTaskLog is not great for user experience.

But the code lgtm.

RichardW98 · 2023-05-24T13:03:33Z

agree. from user experience, we should simplify the code to get context property. I will fix it in next PR.

RichardW98 · 2023-05-24T13:30:03Z

https://issues.redhat.com/browse/FLPATH-403

masayag

Generally speaking, would we like to consider alternative storage to store the data in the DB?

masayag · 2023-05-24T06:42:00Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/enums/LogType.java

+
+public enum LogType {
+
+	TEXT(""), INFO("\u001B[32m"), WARNING("\u001B[33m"), ERROR("\u001B[34m"), END("\u001B[39m");


What is the usage of the code?

to set log level and its ansi code for color

masayag · 2023-05-24T11:06:57Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/enums/LogType.java

+
+import lombok.Getter;
+
+public enum LogType {


LogLevel seems like a common name for this, but I may not get the role of this enum, however, END doesn't fall into that category.
perhaps LogEntryType ?

I will rename to loglevel to comply with standards

masayag · 2023-05-24T11:10:24Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/log/WorkFlowLogService.java

@@ -0,0 +1,11 @@
+package com.redhat.parodos.workflow.log;


seems it should go under
com.redhat.parodos.workflow.log.service package
but perhaps this is okay for the parodos-model-api

ok I will move to log.service and log.dto

masayag · 2023-05-24T13:45:39Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/log/WorkFlowLogService.java

+
+import java.util.UUID;
+
+public interface WorkFlowLogService {


Pls add javadoc. This interface may be in use by workflow developers, It will be useful to have it documented (although the API itself is clear enough)

masayag · 2023-05-24T13:47:00Z

parodos-model-api/src/main/java/com/redhat/parodos/workflow/log/WorkFlowLogService.java

+
+public interface WorkFlowLogService {
+
+	String getLog(UUID workflowExecutionId, String taskName);


seems like the majorify uses workFlowExecutionId (capital F)

yep, naming convention is workFlow***

masayag · 2023-05-24T13:59:55Z

...vice/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowLogServiceImpl.java

+	}
+
+	@Override
+	public void writeLog(UUID mainWorkflowExecutionId, String taskName, WorkFlowTaskLog workFlowTaskLog) {


is it a must to have the main workflow execution id? what about complex workflow where a workunit is a type of workflow: Can logs be added to it as well?

the main workflow execution id here is used to find the workFlowTaskExecution which attached to it. The log is stored on task level which has real execute logic

masayag · 2023-05-24T14:04:35Z

...vice/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowLogServiceImpl.java

+	private WorkFlowTaskExecution getWorkFlowTaskExecution(UUID mainWorkflowExecutionId, String taskName) {
+		WorkFlowTaskDefinition workFlowTaskDefinition = Optional
+				.ofNullable(workFlowTaskDefinitionRepository.findFirstByName(taskName))
+				.orElseThrow(() -> new ResourceNotFoundException("task is not found!"));


Pls extend ResourceType and use

ResourceNotFoundException(ResourceType.WORKFLOW_TASK, IDType.NAME, taskName)

masayag · 2023-05-24T14:05:15Z

...vice/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowLogServiceImpl.java

+		WorkFlowExecution workFlowExecution = Optional
+				.ofNullable(workFlowRepository.findFirstByMainWorkFlowExecutionIdAndWorkFlowDefinitionId(
+						mainWorkflowExecutionId, workFlowTaskDefinition.getWorkFlowDefinition().getId()))
+				.orElseThrow(() -> new ResourceNotFoundException("workflow execution is not found!"));


here as well:

new ResourceNotFoundException(ResourceType.WORKFLOW_EXECUTION, workFlowExecutionId)

masayag · 2023-05-24T14:05:28Z

...vice/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowLogServiceImpl.java

+						workFlowExecution.getId(), workFlowTaskDefinition.getId()))
+				.filter(workFlowTaskExecutions -> !workFlowTaskExecutions.isEmpty())
+				.map(workFlowTaskExecutions -> workFlowTaskExecutions.get(0))
+				.orElseThrow(() -> new ResourceNotFoundException("task execution is not found!"));


masayag · 2023-05-24T14:07:05Z

...rvice/src/main/java/com/redhat/parodos/workflow/execution/controller/WorkFlowController.java

+			@ApiResponse(responseCode = "401", description = "Unauthorized", content = @Content),
+			@ApiResponse(responseCode = "403", description = "Forbidden", content = @Content) })
+	@GetMapping("/{workFlowExecutionId}/{taskName}/log")
+	public ResponseEntity<String> getLog(@PathVariable UUID workFlowExecutionId, @PathVariable String taskName) {


pls add tests to WorkFlowControllerTest

eloycoto · 2023-05-24T15:52:33Z

agree. from user experience, we should simplify the code to get context property. I will fix it in next PR.

TBH, keep creating Jira tickets while we keep accumulating tech debt it's a bad idea IMHO.

RichardW98 · 2023-05-24T16:30:08Z

agree. from user experience, we should simplify the code to get context property. I will fix it in next PR.

TBH, keep creating Jira tickets while we keep accumulating tech debt it's a bad idea IMHO.

ok I will fix the related workcontext properties in this pr 😄 . and leave the rest in next one

pkliczewski

/lgtm

pkliczewski

/lgtm

openshift-ci · 2023-05-26T19:11:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anludke

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [anludke]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from eloycoto and lshannon May 23, 2023 20:07

RichardW98 changed the title ~~workflow log~~ FLPATH-377: workflow log May 23, 2023

openshift-ci-robot added the jira/valid-reference label May 23, 2023

RichardW98 requested a review from wKich May 23, 2023 20:08

RichardW98 force-pushed the FLPATH-377-log branch 3 times, most recently from 519a106 to db31d86 Compare May 24, 2023 02:33

pkliczewski reviewed May 24, 2023

View reviewed changes

masayag reviewed May 24, 2023

View reviewed changes

RichardW98 force-pushed the FLPATH-377-log branch 5 times, most recently from 88a115b to 17925c4 Compare May 24, 2023 22:00

pkliczewski reviewed May 25, 2023

View reviewed changes

openshift-ci bot assigned pkliczewski May 25, 2023

openshift-ci bot added the lgtm label May 25, 2023

pkliczewski added the needs-rebase label May 25, 2023

RichardW98 force-pushed the FLPATH-377-log branch from 17925c4 to 86db601 Compare May 25, 2023 15:08

openshift-ci bot removed the lgtm label May 25, 2023

openshift-merge-robot removed the needs-rebase label May 25, 2023

RichardW98 force-pushed the FLPATH-377-log branch 2 times, most recently from 859383e to 2107a5e Compare May 25, 2023 15:49

RichardW98 force-pushed the FLPATH-377-log branch 4 times, most recently from aee0596 to 8666c50 Compare May 25, 2023 18:21

workflow log

3ad98a3

RichardW98 force-pushed the FLPATH-377-log branch from 8666c50 to 3ad98a3 Compare May 25, 2023 20:52

pkliczewski reviewed May 26, 2023

View reviewed changes

openshift-ci bot added the lgtm label May 26, 2023

anludke approved these changes May 26, 2023

View reviewed changes

openshift-ci bot added the approved label May 26, 2023

openshift-merge-robot merged commit bdbdd31 into parodos-dev:main May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLPATH-377: workflow log #368

FLPATH-377: workflow log #368

RichardW98 commented May 23, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 23, 2023 •

edited by openshift-ci bot

Loading

pkliczewski May 24, 2023

pkliczewski May 24, 2023

RichardW98 May 24, 2023

pkliczewski May 24, 2023

RichardW98 May 24, 2023

eloycoto commented May 24, 2023

RichardW98 commented May 24, 2023

RichardW98 commented May 24, 2023

masayag left a comment

masayag May 24, 2023

RichardW98 May 24, 2023

masayag May 24, 2023

RichardW98 May 24, 2023 •

edited

Loading

masayag May 24, 2023

RichardW98 May 24, 2023

masayag May 24, 2023

masayag May 24, 2023

RichardW98 May 24, 2023

masayag May 24, 2023

RichardW98 May 24, 2023

masayag May 24, 2023

masayag May 24, 2023

masayag May 24, 2023

masayag May 24, 2023

eloycoto commented May 24, 2023

RichardW98 commented May 24, 2023

pkliczewski left a comment

pkliczewski left a comment

openshift-ci bot commented May 26, 2023


		public enum LogType {

		TEXT(""), INFO("\u001B[32m"), WARNING("\u001B[33m"), ERROR("\u001B[34m"), END("\u001B[39m");


		public interface WorkFlowLogService {

		String getLog(UUID workflowExecutionId, String taskName);

FLPATH-377: workflow log #368

FLPATH-377: workflow log #368

Conversation

RichardW98 commented May 23, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented May 23, 2023 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eloycoto commented May 24, 2023

RichardW98 commented May 24, 2023

RichardW98 commented May 24, 2023

masayag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardW98 May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eloycoto commented May 24, 2023

RichardW98 commented May 24, 2023

pkliczewski left a comment

Choose a reason for hiding this comment

pkliczewski left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 26, 2023

RichardW98 commented May 23, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 23, 2023 •

edited by openshift-ci bot

Loading

RichardW98 May 24, 2023 •

edited

Loading