Versioning Metrics: pt1 #7822
Versioning Metrics: pt1 #7822
Conversation
Shivs11
force-pushed
the
shahab/versioning-metrics
branch
from
34253fa to
cb580e2
Compare
| return nil, consts.ErrQueryEnteredInvalidState | ||
| } | ||
| case workflow.QueryCompletionTypeUnblocked: | ||
| msResp, err := api.GetMutableState(ctx, shardContext, workflowKey, workflowConsistencyChecker) |
There was a problem hiding this comment.
not sure pulling this call out is better :) I was suggesting to keep this part untouched. in line 182 you can get the behavior from the mutableState that we already have. (and then the behavior directly to emitWorkflowQueryMetrics)
There was a problem hiding this comment.
If I extract behaviour outside, then this block of code (which I have now restored) won't get the chance to pass the most up-to-date behaviour
what I have now done is just pass msResp according to the scope it's in
There was a problem hiding this comment.
why not? it can pass workflow.GetEffectiveVersioningBehavior(msResp.GetVersioningInfo()) instead of the old behavior
ShahabT
left a comment
ShahabT
left a comment
There was a problem hiding this comment.
approved with a minor comment
| emitWorkflowQueryMetrics( | ||
| metricsHandler, | ||
| nsEntry, | ||
| msResp, |
There was a problem hiding this comment.
Nit: I still think the function should just get a behavior and we should pass workflow.GetEffectiveVersioningBehavior(msResp.GetVersioningInfo()) in here.
Then line 183 will just be behavior := mutableState.GetEffectiveBehavior() instead of msResp, err := api.MutableStateToGetResponse(mutableState)
There was a problem hiding this comment.
i thought about that too, but inside emitWorkflowQueryMetrics we are using msResp to generate two different tags - one for versioningBehaviour, the other for WorkflowStatus tag. Sure, one could argue we could just pass these two as separate arguments but I think it's easier to read when not done so - moreover, if we were to increase the number of tags on this metric, there's a high chance those new tags might come from a MS field and it'll be nice to have msResp be present.
What changed?
Metrics added in this PR are:
(Directly related to entity workflows)
(Not directly related to entity workflows)
Why?
How did you test it?
Potential risks