Versioning Metrics pt2: DeploymentTransition + VersioningOverride #7825
Conversation
Shivs11
changed the title
| } | ||
|
|
||
| // Versioning Override set via UpdateWorkflowExecutionOptionsRequest | ||
| if versioningOverride != nil { |
There was a problem hiding this comment.
the API may be called for other things too. let's only emit the metric if effective behavior OR effective version changes.
| metrics.NamespaceTag(ms.namespaceEntry.Name().String()), | ||
| metrics.VersioningBehaviorBeforeOverrideTag(enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED), | ||
| metrics.VersioningBehaviorAfterOverrideTag(startRequest.GetStartRequest().GetVersioningOverride().GetBehavior()), //nolint:staticcheck // SA1019: worker versioning v0.31 | ||
| metrics.VersioningOverrideOnNewWorkflowTag(prevRunID == ""), |
There was a problem hiding this comment.
it'd be super cool if instead we could have the following values:
_new, _child, _can, _retry, _cron. For the last three you can look at the initiator field.
Add a _existing to the list to cover the case of UpdateOptions.
There was a problem hiding this comment.
let me know if you like with what I just came up with
| metrics.EffectiveDeploymentBeforeTransitionTag(worker_versioning.ExternalWorkerDeploymentVersionToString(worker_versioning.ExternalWorkerDeploymentVersionFromDeployment(preTransitionEffectiveDeployment))), | ||
| metrics.EffectiveDeploymentAfterTransitionTag(worker_versioning.ExternalWorkerDeploymentVersionToString(worker_versioning.ExternalWorkerDeploymentVersionFromDeployment(deployment))), | ||
| ), |
There was a problem hiding this comment.
I think here we just want two bools: from_unversioned=true/false, to_unversioned=true/false
common/metrics/tags.go
Outdated
| if prevRunID == "" { | ||
| return &tagImpl{key: runInitiator, value: newRun} | ||
| } else if attributes.GetInitiator() == enumspb.CONTINUE_AS_NEW_INITIATOR_WORKFLOW { | ||
| return &tagImpl{key: runInitiator, value: childRun} |
There was a problem hiding this comment.
| return &tagImpl{key: runInitiator, value: childRun} | |
| return &tagImpl{key: runInitiator, value: canRun} |
common/metrics/tags.go
Outdated
| newRun = "__new" | ||
| existingRun = "__existing" | ||
| childRun = "__child" | ||
| canRun = "__can" | ||
| retryRun = "__retry" | ||
| cronRun = "__cron" |
There was a problem hiding this comment.
actually, I don't think we need the __ prefixes for this values, it's not like these are special or place-holder values, these are all the values that the tag can get.
| metrics.FromUnversionedTag(worker_versioning.ExternalWorkerDeploymentVersionToString(worker_versioning.ExternalWorkerDeploymentVersionFromDeployment(preTransitionEffectiveDeployment))), | ||
| metrics.ToUnversionedTag(worker_versioning.ExternalWorkerDeploymentVersionToString(worker_versioning.ExternalWorkerDeploymentVersionFromDeployment(deployment))), |
There was a problem hiding this comment.
why not have FromUnversionedTag get a deployment directly? it's nice to not introduce new code that uses strings.
There was a problem hiding this comment.
Passing the deployment as is to the tags file and letting that be responsible for converting the deployment into a string doesn't sound like it's responsibility - moreover, passing in deployment to tags and calling worker_versioning package also didn't work since it introduced a cyclic chain of imports!
What changed?
Why?
How did you test it?
Potential risks