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.

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

Calculate workflow history size and count and expose that to client #1270

Merged
merged 30 commits into from
Nov 15, 2023

Conversation

timl3136
Copy link
Contributor

@timl3136 timl3136 commented Sep 1, 2023

What changed?
Add fields exposing workflow history size and count to client in form of api calls. Client side now will calculate the history size of a workflow while processing events using size of the immutable fields and a constant buffer to account for smaller fields that might change to avoid non-determinism.
Combined with server change that emits history data to client #5392 that will be used to compare against. now customers can access their workflow history data and decide whether creating a ContinueAsNew workflow base on that information.

Why?
Due to Cadence history limit, many customers find their workflow being terminated and there is currently no mechanisms for customer to know their workflow's history usage.
Resolves issues #917

How did you test it?
The code is tested pairing with server code implementation on development environment

Potential risks
In the worst case, the server might not have the latest update with transmitting the history data to client. In that case, the sanity check (not yet implemented) might failed.

@timl3136 timl3136 changed the title Expose history Expose workflow history size and count to client Sep 2, 2023
@timl3136 timl3136 force-pushed the expose-history branch 3 times, most recently from d838f8d to c2a5c26 Compare September 6, 2023 23:41
Add logger to get whole history

sync js change in idl and remove logger

add history size and logger into task handlers

Update reference to cadence-idl, sync field name with idl repo

update idl ref
@timl3136 timl3136 changed the title Expose workflow history size and count to client Calculate workflow history size and count and expose that to client Oct 24, 2023
internal/internal_event_handlers.go Outdated Show resolved Hide resolved
internal/internal_event_handlers.go Outdated Show resolved Hide resolved
internal/internal_event_handlers.go Outdated Show resolved Hide resolved
internal/internal_event_handlers.go Outdated Show resolved Hide resolved
internal/internal_task_pollers.go Outdated Show resolved Hide resolved
@@ -1379,3 +1384,139 @@ func (weh *workflowExecutionEventHandlerImpl) handleSignalExternalWorkflowExecut

return nil
}

func (weh *workflowExecutionEventHandlerImpl) estimateHistorySize(event *m.HistoryEvent) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't need to be attached to *workflowExecutionEventHandlerImpl. it only uses weh.logger which can be passed as argument. So please make this a standalone function and move to internal_utils.go as well as other helper functions you added below. This file is already huge (like many others in internal package unfortunately) and we will slowly refactor each type into its own file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in latest commit

internal/internal_event_handlers.go Outdated Show resolved Hide resolved
workflow/workflow.go Show resolved Hide resolved
@timl3136 timl3136 merged commit c20b993 into uber-go:master Nov 15, 2023
9 checks passed
timl3136 added a commit to timl3136/cadence-client that referenced this pull request Nov 16, 2023
…ber-go#1270)

Enable client side estimated history size exposure via API
@petergardfjall
Copy link

Hi, after upgrading to go.uber.org/cadence v1.2.6 we are seeing some log warnings that seem to stem from this PR.

2023-12-04T12:21:28.005+0100	WARN	internal/internal_utils.go:473	unknown event type { ... "Event Type": "TimerFired"}
go.uber.org/cadence/internal.estimateHistorySize
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_utils.go:473
go.uber.org/cadence/internal.(*workflowExecutionEventHandlerImpl).ProcessEvent
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_event_handlers.go:945
go.uber.org/cadence/internal.(*workflowExecutionContextImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:917
go.uber.org/cadence/internal.(*workflowTaskHandlerImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:805
go.uber.org/cadence/internal.(*workflowTaskPoller).processWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:344
go.uber.org/cadence/internal.(*workflowTaskPoller).ProcessTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:317
go.uber.org/cadence/internal.(*baseWorker).processTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_worker_base.go:380

Similarly for ChildWorkflowExecutionStarted:

2023-12-04T12:25:44.870+0100	WARN	internal/internal_utils.go:473	unknown event type { ... "Event Type": "ChildWorkflowExecutionStarted"}
go.uber.org/cadence/internal.estimateHistorySize
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_utils.go:473
go.uber.org/cadence/internal.(*workflowExecutionEventHandlerImpl).ProcessEvent
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_event_handlers.go:945
go.uber.org/cadence/internal.(*workflowExecutionContextImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:917
go.uber.org/cadence/internal.(*workflowTaskHandlerImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:805
go.uber.org/cadence/internal.(*workflowTaskPoller).processWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:344
go.uber.org/cadence/internal.(*workflowTaskPoller).ProcessTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:317
go.uber.org/cadence/internal.(*baseWorker).processTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_worker_base.go:380

Is this just a case of the added estimateHistorySize not covering all possible events or is it something for us to be concerned about?

@taylanisikdemir
Copy link
Contributor

Hi, after upgrading to go.uber.org/cadence v1.2.6 we are seeing some log warnings that seem to stem from this PR.

2023-12-04T12:21:28.005+0100	WARN	internal/internal_utils.go:473	unknown event type { ... "Event Type": "TimerFired"}
go.uber.org/cadence/internal.estimateHistorySize
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_utils.go:473
go.uber.org/cadence/internal.(*workflowExecutionEventHandlerImpl).ProcessEvent
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_event_handlers.go:945
go.uber.org/cadence/internal.(*workflowExecutionContextImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:917
go.uber.org/cadence/internal.(*workflowTaskHandlerImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:805
go.uber.org/cadence/internal.(*workflowTaskPoller).processWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:344
go.uber.org/cadence/internal.(*workflowTaskPoller).ProcessTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:317
go.uber.org/cadence/internal.(*baseWorker).processTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_worker_base.go:380

Similarly for ChildWorkflowExecutionStarted:

2023-12-04T12:25:44.870+0100	WARN	internal/internal_utils.go:473	unknown event type { ... "Event Type": "ChildWorkflowExecutionStarted"}
go.uber.org/cadence/internal.estimateHistorySize
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_utils.go:473
go.uber.org/cadence/internal.(*workflowExecutionEventHandlerImpl).ProcessEvent
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_event_handlers.go:945
go.uber.org/cadence/internal.(*workflowExecutionContextImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:917
go.uber.org/cadence/internal.(*workflowTaskHandlerImpl).ProcessWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_handlers.go:805
go.uber.org/cadence/internal.(*workflowTaskPoller).processWorkflowTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:344
go.uber.org/cadence/internal.(*workflowTaskPoller).ProcessTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_task_pollers.go:317
go.uber.org/cadence/internal.(*baseWorker).processTask
	/home/peterg/go/pkg/mod/go.uber.org/cadence@v1.2.6/internal/internal_worker_base.go:380

Is this just a case of the added estimateHistorySize not covering all possible events or is it something for us to be concerned about?

That's right. It's being addressed in #1300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants