ProcessInstanceInfo set start date when constructed #1705
Conversation
In the `PrometheusProcessEventListener`, the field `startDate` from a `processInstance` is used to time the process instance duration metric. https://github.com/kiegroup/droolsjbpm-integration/blob/4fb9e4c239dfc21ea99ea5b701877cafeafcbe1d/kie-server-parent/kie-server-services/kie-server-services-prometheus/src/main/java/org/kie/server/services/prometheus/PrometheusProcessEventListener.java#L96 However, the start date was always null because it is being set when `getProcessInstance` is first called. Since `getProcessInstance` is not called before we trigger the event listener, the start date was never set. The `ProcessInstance` start date should be set when the `ProcessInstanceInfo` object is first created.
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
9 similar comments
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
|
ok to test |
There was a problem hiding this comment.
i believe we just need to have constructor in WorkflowProcessInstanceImpl() which sets start date to current.
public WorkflowProcessInstanceImpl() {
this.startDate = new Date();
}
Tested on my local and prometheus metrics were enabled for "kie_server_process_instance_duration_seconds Kie Server Process Instances Duration"
|
jenkins retest this please |
Yes this will work. However I still believe that more appropriate way will be to have start date in the object all the time :-) |
|
jenkins retest this please |
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
@hmatt1 thanks a lot for your contribution!
Some comments regarding some stuff that can be removed or moved to a better place, but looks good.
Could you add also a test to check startDates are the same when creating a second ProcessInfo for the same process instance?
...-persistence-jpa/src/main/java/org/jbpm/persistence/processinstance/ProcessInstanceInfo.java
Outdated
Show resolved
Hide resolved
...-persistence-jpa/src/main/java/org/jbpm/persistence/processinstance/ProcessInstanceInfo.java
Outdated
Show resolved
Hide resolved
...-persistence-jpa/src/main/java/org/jbpm/persistence/processinstance/ProcessInstanceInfo.java
Outdated
Show resolved
Hide resolved
The twoProcessInstanceInfoSameStartDateTest was added along with other minor changes based on review comments
|
Thank you @gmunozfe @deekshajohari and @bxf12315 for the review comments! I updated the changes based on @gmunozfe's comments. I'm happy to refactor again if we want to go with what @deekshajohari suggested, to move initializing the |
I removed these comments since we now have a test for it. |
|
jenkins retest this please |
There was a problem hiding this comment.
Hi @hmatt1
Unfortunately I cannot accept this as a fix because the next reasons:
Just some context. The process instance info contains the process instance (just a marshalling of this one) and the relation between one or another is 1:1. Actually once the process instance is completed the process instance info is removed as it is runtime information and there is no need anymore. so the process instance info is created when the process instance is created so its start date means the date when the process is created. I said that because in your test
twoProcessInstanceInfoSameStartDateTest you are setting the two process instance info for the same process instance.
ProcessInstanceInfo start date show when the process instance is created and not when is started. (those are two different things)
WorkflowProcessInstanceImpl start date show when the process is started. While you are right that is not set properly IMO you
should remove the logic you added.
The only thing you need to add here is
https://github.com/kiegroup/jbpm/blob/master/jbpm-flow/src/main/java/org/jbpm/workflow/instance/impl/WorkflowProcessInstanceImpl.java#L494
this.startDate = new Date();
this should fix the issue. Keep in mind that processInstanceInfo startDate and workprocessInstanceimpl could have different start dates but it should guaranteed that processInstanceInfo startDate <= workflowprocessinstanceimpl startdate
Would you mind to fix your PR logic and tests ?
PS. thx for catching this problem. good work !
Set the start date when `start()` is called
|
Thanks @elguardian for the review! This approach is much simpler 😄 Thanks for the background about process instance info too |
There was a problem hiding this comment.
Looks good also to me. Thanks for your contribution @hmatt1 !
This is ready to merge @elguardian
This is my first PR against jbpm 😄 Please let me know if I should open a Jira ticket here for this fix.
In the
PrometheusProcessEventListener, the fieldstartDatefrom aprocessInstanceis used to time the process instance duration metric.https://github.com/kiegroup/droolsjbpm-integration/blob/4fb9e4c239dfc21ea99ea5b701877cafeafcbe1d/kie-server-parent/kie-server-services/kie-server-services-prometheus/src/main/java/org/kie/server/services/prometheus/PrometheusProcessEventListener.java#L96
However, the start date was always null because it is being set when
getProcessInstanceis first called.Since
getProcessInstanceis not called before we trigger the event listener, the start date was never set.The
ProcessInstancestart date should be set when theProcessInstanceInfoobject is first created.