Make sure workflow_failed is incremented on NonDeterministicException

[Quinn-With-Two-Ns]

Make sure workflow_failed is incremented on NonDeterministicException

closes #2103

[cretz]

I see completeWorkflow increments this metric. Do these exceptions not make it to completeWorkflow? Is there a common place that completeWorkflow-based exceptions and these are turned to workflow failures that may be a better place to move the metric, e.g. WorkflowStateMachines.failWorkflow? Or even if we don't want to change completeWorkflow, is there a common place where WorkflowExecutionException is caught and converted?

[Quinn-With-Two-Ns]

Do these exceptions not make it to completeWorkflow

Correct, this code catches exceptions from the state machine, not the workflow code itself

Is there a common place that completeWorkflow-based exceptions and these are turned to workflow failures that may be a better place to move the metric, e.g. WorkflowStateMachines.failWorkflow

No

is there a common place where WorkflowExecutionException is caught and converted?

The only place would be when the SDK responds to the server, but that would require changing where all completion metrics are emitted. That work is captured here #1590. I was hopping to defer that work till later.

[cretz]

Curious, what is the situation where e is a WorkflowExecutionException here?

[Quinn-With-Two-Ns]

I don't think it can happen today, but if workflowStateMachines.handleEvent(event, hasNext); did throw a WorkflowExecutionException it would lead to a workflow failure. So it is more future proofing, it also may be possible with some user interceptor could throw a WorkflowExecutionException.

[cretz]

Works for me.

[cretz]

LGTM. I feel better knowing we are asserting exactly 1 and so we will make sure dupe fail counts don't creep in if/when we refactor to a common place.