Add log and metrics to workflow termination events #6146
Conversation
fimanishi
force-pushed
the
metrics-for-workflows-affected-by-conflict-resolution
branch
3 times, most recently
from
197331d
to
f3a7f5d
Compare
fimanishi
force-pushed
the
metrics-for-workflows-affected-by-conflict-resolution
branch
3 times, most recently
from
5329e75
to
08460ec
Compare
Pull Request Test Coverage Report for Build 019050ae-cea2-4903-9de4-b1001335191cDetails
💛 - Coveralls |
fimanishi
force-pushed
the
metrics-for-workflows-affected-by-conflict-resolution
branch
from
08460ec
to
2f2f9c7
Compare
Pull Request Test Coverage Report for Build 019060fb-a2f5-46ac-a87c-29c4b9ea3cefDetails
💛 - Coveralls |
fimanishi
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -47,5 +54,19 @@ func TerminateWorkflow( | |||
| terminateDetails, | |||
| terminateIdentity, | |||
| ) | |||
|
|
|||
| domainName := mutableState.GetDomainEntry().GetInfo().Name | |||
There was a problem hiding this comment.
I suggest moving these metrics to AddWorkflowExecutionTerminatedEvent. It will simplify things since mutableStateBuilder already has metrics.Client and logger. Then you don't need to change the signature of this method
| tag.WorkflowDomainName(domainName), | ||
| tag.WorkflowID(r.mutableState.GetExecutionInfo().WorkflowID), | ||
| tag.WorkflowRunID(r.mutableState.GetExecutionInfo().RunID), | ||
| tag.WorkflowTerminationReason(WorkflowTerminationReason), |
There was a problem hiding this comment.
is this code path only called for conflict based terminations?
There was a problem hiding this comment.
yes, it's only called here
cadence/service/history/execution/workflow.go
Line 201 in 2f2f9c7
There was a problem hiding this comment.
I recommend passing the reason as a parameter because this looks like a helper function that may get called from other places in the future. Having a reason param will force the future changes to pass the actual reason which is desired compared to logging the invalid reason for such cases.
fimanishi
force-pushed
the
metrics-for-workflows-affected-by-conflict-resolution
branch
from
8e519a7
to
7aa89ca
Compare
| tag.WorkflowTerminationReason(reason), | ||
| ) | ||
|
|
||
| scopeWithDomainTag := e.metricsClient.Scope(metrics.HistoryTerminateWorkflowExecutionScope).Tagged(metrics.DomainTag(domainName)) |
There was a problem hiding this comment.
Perhaps we can add the reason dimension to this?
fimanishi
force-pushed
the
metrics-for-workflows-affected-by-conflict-resolution
branch
3 times, most recently
from
c034d9b
to
460c452
Compare
**What changed?** Added logs to workflow termination events, containing the reason for the termination, tagged with the `domainName`, `workflowID`, `terminationReason`, and `runID`. Added metrics to workflow termination events, using a counter per domain `WorkflowTerminateCounterPerDomain` under the `HistoryTerminateWorkflowExecutionScope` scope, with `WorkflowTerminationReasonTag` **Why?** Improve workflow termination visibility, allowing Cadence and clients to easily find terminated workflows. This is particularly important to provide better information for workflows terminated during failovers. **How did you test it?** Unit tests. **Potential risks** The risks are associated with the changes in functions parameters being passed. Need to ensure that the parameters are correct and that they do not contain `nil` values. **Release notes** **Documentation Changes**
fimanishi
force-pushed
the
metrics-for-workflows-affected-by-conflict-resolution
branch
from
460c452
to
e3becf7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…WorkflowTerminationReasonTag`.
| tag.WorkflowTerminationReason(reason), | ||
| ) | ||
|
|
||
| re := regexp.MustCompile(`[^a-zA-Z0-9]`) |
There was a problem hiding this comment.
sorry to be annoying, but lets move this outside the function invocation, there's no need to recompile it each time.
By convention, go programs put contstants, type declarations up the top, so perhaps around the top of the metrics/tags.go we can define the regex and then just use it inside the metrics.WorkflowTerminationReasonTag function, thefore we don't need to remember to sanitize inline like this for every use?
The syntax outside function closure is slightly different, so either
var safeAlphaNumericStringRE = regexp.MustCompile(`[^a-zA-Z0-9]`)
// or, within a var block like:
var (
safeAlphaNumericStringRE = regexp.MustCompile(`[^a-zA-Z0-9]`)
)
and that will make it easier such that we reference the already compiled regex
// WorkflowTerminationReasonTag reports the reason for workflow termination
func WorkflowTerminationReasonTag(value string) Tag {
value = safeAlphaNumericStringRE.ReplaceAllString(value, "_")
return simpleMetric{key: workflowTerminationReason, value: value}
}
…unction to avoid performance issues
fimanishi
deleted the
metrics-for-workflows-affected-by-conflict-resolution
branch
What changed?
Added logs to workflow termination events, containing the reason for the termination, tagged with the
domainName,workflowID,terminationReason, andrunID.Added metrics to workflow termination events, using a counter per domain
WorkflowTerminateCounterPerDomainunder theHistoryTerminateWorkflowExecutionScopescope.Why?
Improve workflow termination visibility, allowing Cadence and clients to easily find terminated workflows. This is particularly important to provide better information for workflows terminated during failovers.
How did you test it?
Unit tests.
Potential risks
The risks are associated with the changes in functions parameters being passed. Need to ensure that the parameters are correct and that they do not contain
nilvalues.Release notes
Documentation Changes