Cleanup CRE metics, dedupe accidental duplication and remove unnecessary if guard on label#21352
Cleanup CRE metics, dedupe accidental duplication and remove unnecessary if guard on label#21352
Conversation
…ary if guard on label
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
product-security-plaid-production
bot
requested a review
from fernandofederico1984
There was a problem hiding this comment.
Pull request overview
Cleans up workflow engine (CRE) observability by simplifying label construction and removing duplicate/unused metrics instruments and increments.
Changes:
- Simplifies engine label construction and makes the SDK label consistently included in logger/metrics label sets.
- Removes the legacy
platform_engine_subscriptions/platform_engine_executionscounters and their increment sites. - Removes the corresponding unit test that only exercised the deleted counters.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/services/workflows/v2/engine.go | Simplifies label building, always includes SDK label, and removes duplicate subscription/execution counter increments. |
| core/services/workflows/monitoring/monitoring.go | Deletes unused subscriptions/executions counters and associated labeler methods. |
| core/services/workflows/monitoring/monitoring_test.go | Removes the test for the deleted counters and cleans up imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -706,7 +700,6 @@ func (e *Engine) startExecution(ctx context.Context, wrappedTriggerEvent enqueue | |||
| e.lggr.Errorf("failed to ACK trigger event (eventID=%s): %v", triggerEvent.ID, err) | |||
| } | |||
| e.metrics.With("workflowID", e.cfg.WorkflowID, "workflowName", e.cfg.WorkflowName.String()).IncrementWorkflowExecutionStartedCounter(ctx) | |||
There was a problem hiding this comment.
e.metrics is already constructed in NewEngine() with workflowID, workflowOwner, workflowName, and sdk labels. The extra With("workflowID", ..., "workflowName", ...) here is redundant (and uses string literals instead of platform.Key* constants), which adds avoidable allocations and makes label keys easier to drift. Consider dropping the With(...) call entirely (or at least switching to platform.KeyWorkflowID / platform.KeyWorkflowName).
| e.metrics.With("workflowID", e.cfg.WorkflowID, "workflowName", e.cfg.WorkflowName.String()).IncrementWorkflowExecutionStartedCounter(ctx) | |
| e.metrics.IncrementWorkflowExecutionStartedCounter(ctx) |
|
|
Hi team, I'll help clean up the CRE metrics and remove the accidental duplication. I'll analyze the codebase and ensure a clean implementation. My approach:
Please assign! |
| c.em.subscriptions.Add(ctx, 1, metric.WithAttributes(otelLabels...)) | ||
| } | ||
|
|
||
| func (c WorkflowsMetricLabeler) IncrementExecutionsCounter(ctx context.Context) { |
There was a problem hiding this comment.
Out of scope now, but why are all of the methods declared with value receivers instead of pointers? Seems like a lot of unnecessary memory allocs. We even return a pointer from the constructor.
No description provided.