Add archetype metric tag and logical task processing metrics#9377
Add archetype metric tag and logical task processing metrics#9377awln-temporal merged 15 commits intomainfrom
Conversation
| } | ||
|
|
||
| // Execute all fired pure tasks for a component while holding the workflow lock. | ||
| archetypeTag := getArchetypeTagForChasmTask(task.GetArchetypeID(), t.shardContext.ChasmRegistry()) |
There was a problem hiding this comment.
nit: we can reuse getArchetypeTag defined above?
| if err == nil { | ||
| processedTimers += 1 | ||
|
|
||
| metrics.ChasmPureTaskRequests.With(t.metricsHandler).Record(1, archetypeTag) |
There was a problem hiding this comment.
we need to tag the pure task type as well right?
I was originally think we can do this in chasmTree.ExecutePureTask/ValidatePureTask method where the logic already looked up the registrable task based on the task instance.
There was a problem hiding this comment.
moved it to the chasm framework, with metrics handler as an arg to ExecutePureTask. Immediate pure tasks will not have physical task type tag, but will still include namespace, pure task type, and archetype tag. Scheduled pure tasks via physical task queues will still have the same tags applied plus the new archetype and pure task type tag.
service/history/queues/executable.go
Outdated
There was a problem hiding this comment.
ah nice. I was originally thinking we need to change every task executor implementation to return the new archetype tag. But make sense since this is called in the executable constructor and we basically a set of base tags there and archetype never changes.
We should probably rename this to baseTaskMetricTags
There was a problem hiding this comment.
renamed to taskBaseMetricTags since baseTask is a bit different in my head
service/history/queues/executable.go
Outdated
| @@ -89,6 +89,7 @@ var defaultExecutableMetricsTags = []metrics.Tag{ | |||
There was a problem hiding this comment.
I feel we no longer need this since NewExecutable will always initialize the metricHandler with the necessary tags?
There was a problem hiding this comment.
yeah there's no fallbacks, all the tags will exist, removed this and all references.
There was a problem hiding this comment.
hmm looks like changes for this file is not commited/pushed? I can still see defaultExecutableMetricsTags and estimateTaskMetricTags is not renamed.
There was a problem hiding this comment.
pushed now, sorry about that
bd29336 to
bacead3
Compare
chasm/tree.go
Outdated
|
|
||
| // Only syncStructure on next iteration if task is executed (the first return value). | ||
| syncStructure, err = taskNode.ExecutePureTask(context.Background(), task.attributes, task.task) | ||
| syncStructure, err = taskNode.ExecutePureTask(context.Background(), handler, task.attributes, task.task) |
There was a problem hiding this comment.
oh good catch! I already forgot we have this special immediate pure task logic 🤦
| return processedTimers, nil | ||
| } | ||
|
|
||
| func getArchetypeTagForChasmTask(archetypeID chasm.ArchetypeID, chasmRegistry *chasm.Registry) metrics.Tag { |
service/history/queues/executable.go
Outdated
| @@ -89,6 +89,7 @@ var defaultExecutableMetricsTags = []metrics.Tag{ | |||
There was a problem hiding this comment.
hmm looks like changes for this file is not commited/pushed? I can still see defaultExecutableMetricsTags and estimateTaskMetricTags is not renamed.
ab6b973 to
ce8a938
Compare
service/history/queues/executable.go
Outdated
| executed, err := executor.ExecutePureTask(ctx, t.metricsHandler, taskAttributes, taskInstance) | ||
| if err == nil && executed { | ||
| processedTimers++ |
There was a problem hiding this comment.
revert?
Also I don't think we can have the executed check. processedTimers is probably not a good name, it's essentially for deciding if we need to do a write operation. If we have any invalid tasks, they won't be executed but we still want to a write op to delete them and create the next pure task (part of the closeTransaction logic).
e559a32 to
4176e06
Compare
f494b38 to
5b72162
Compare
5b72162 to
f4f0bc2
Compare
dandavison
left a comment
There was a problem hiding this comment.
I approve the one line change in chasm/lib/callback/tasks_test.go. I haven't otherwise reviewed the PR.
…lio#9377) ## What changed? Add archetype metric tag and logical task processing metrics, and add `chasm_pure_task_requests` and `chasm_pure_task_errors` metrics. Archetype metric tab will be applied for all tasks executed, physical and logical, that get added in Executable.go by default. If the archetype is found, the archetype display name will be used for the tag value. Otherwise, will default to `workflow` archetype. For chasm logical pure task metrics, will resolve to `__unknown__` if archetype is not found, since this would be unexpected behavior. ## Why? Components and therefore task types can be used across different archetype executions. In the future with more archetypes, there needs to be visibility for which archetype created the task for execution. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [ ] added new functional test(s)
…lio#9377) ## What changed? Add archetype metric tag and logical task processing metrics, and add `chasm_pure_task_requests` and `chasm_pure_task_errors` metrics. Archetype metric tab will be applied for all tasks executed, physical and logical, that get added in Executable.go by default. If the archetype is found, the archetype display name will be used for the tag value. Otherwise, will default to `workflow` archetype. For chasm logical pure task metrics, will resolve to `__unknown__` if archetype is not found, since this would be unexpected behavior. ## Why? Components and therefore task types can be used across different archetype executions. In the future with more archetypes, there needs to be visibility for which archetype created the task for execution. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [ ] added new functional test(s)
## What changed? Add archetype metric tag and logical task processing metrics, and add `chasm_pure_task_requests` and `chasm_pure_task_errors` metrics. Archetype metric tab will be applied for all tasks executed, physical and logical, that get added in Executable.go by default. If the archetype is found, the archetype display name will be used for the tag value. Otherwise, will default to `workflow` archetype. For chasm logical pure task metrics, will resolve to `__unknown__` if archetype is not found, since this would be unexpected behavior. ## Why? Components and therefore task types can be used across different archetype executions. In the future with more archetypes, there needs to be visibility for which archetype created the task for execution. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [ ] added new functional test(s)
…lio#9377) ## What changed? Add archetype metric tag and logical task processing metrics, and add `chasm_pure_task_requests` and `chasm_pure_task_errors` metrics. Archetype metric tab will be applied for all tasks executed, physical and logical, that get added in Executable.go by default. If the archetype is found, the archetype display name will be used for the tag value. Otherwise, will default to `workflow` archetype. For chasm logical pure task metrics, will resolve to `__unknown__` if archetype is not found, since this would be unexpected behavior. ## Why? Components and therefore task types can be used across different archetype executions. In the future with more archetypes, there needs to be visibility for which archetype created the task for execution. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [ ] added new functional test(s)
What changed?
Add archetype metric tag and logical task processing metrics, and add
chasm_pure_task_requestsandchasm_pure_task_errorsmetrics. Archetype metric tab will be applied for all tasks executed, physical and logical, that get added in Executable.go by default. If the archetype is found, the archetype display name will be used for the tag value. Otherwise, will default toworkflowarchetype.For chasm logical pure task metrics, will resolve to
__unknown__if archetype is not found, since this would be unexpected behavior.Why?
Components and therefore task types can be used across different archetype executions. In the future with more archetypes, there needs to be visibility for which archetype created the task for execution.
How did you test it?