fix: enforce metrics attributes, also fixing one doc #14979
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR enforces type-safe metrics attributes by generating helper functions that ensure all builtin metrics use the correct attributes as documented in values.yaml. The discovery of issue #14976 prompted this enforcement mechanism. The PR also fixes missing documentation for the phase attribute on the workflowtemplate_triggered_total metric.
- Generated type-safe helper methods for all builtin metrics that enforce correct attribute usage
- Replaced direct instrument API calls with generated helper methods throughout the codebase
- Fixed documentation to include the missing
phaseattribute onworkflowtemplate_triggered_totalmetric
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/metrics/work_queue.go | Refactored to use type-safe helper methods instead of direct instrument calls |
| workflow/metrics/metrics_k8s_request.go | Updated to use generated helper methods for K8s request metrics |
| workflow/metrics/leader.go | Replaced direct gauge observation with generated helper |
| workflow/metrics/histogram_template.go | Updated to use generated helper for workflow template runtime recording |
| workflow/metrics/histogram_durations.go | Updated to use generated helper for operation duration recording |
| workflow/metrics/gauge_workflow_phase.go | Replaced direct gauge observation with generated helper |
| workflow/metrics/gauge_workflow_condition.go | Updated to use generated helper for workflow condition observation |
| workflow/metrics/gauge_pod_phase.go | Updated to use generated helper for pod phase observation |
| workflow/metrics/counter_workflow_phase.go | Updated to use generated helper for workflow phase counting |
| workflow/metrics/counter_template.go | Removed manual attribute building in favor of generated helper |
| workflow/metrics/counter_pod_phase.go | Updated to use generated helper for pod phase counting |
| workflow/metrics/counter_pod_pending.go | Updated to use generated helper for pod pending counting |
| workflow/metrics/counter_pod_missing.go | Updated to use generated helper for pod missing counting |
| workflow/metrics/counter_log.go | Refactored to use generated helper and atomic helper struct pattern |
| workflow/metrics/counter_error.go | Updated to use generated helper for error counting |
| workflow/metrics/counter_cronworkflow_trigger.go | Updated to use generated helper for cron workflow trigger counting |
| workflow/metrics/counter_cronworkflow_policy.go | Updated to use generated helper for cron workflow policy counting |
| util/telemetry/version.go | Updated to use generated helper for version metric |
| util/telemetry/metrics_helpers.go | Generated file containing type-safe helper methods for all metrics |
| util/telemetry/counter_deprecations.go | Updated to use generated helper with optional attributes pattern |
| util/telemetry/builder/values.yaml | Added missing phase attribute to workflowtemplate_triggered_total and type annotations |
| util/telemetry/builder/metrics_helpers.go | Generator code for creating type-safe metrics helper methods |
| util/telemetry/builder/builder.go | Added support for attribute types and helper generation |
| docs/metrics.md | Fixed documentation to include missing phase attribute |
| Makefile | Added build target for generating metrics_helpers.go |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Joibel
force-pushed
the
claude/optimize-metric-attributes-011CUQBVWgg2ccfmAWp7QbhA
branch
2 times, most recently
from
5443ac4 to
f31e259
Compare
…tion Implement a code generation system that creates type-safe wrapper methods for all OpenTelemetry metrics, enforcing correct attribute usage at compile time without runtime overhead. Key Features: - Generate type-safe helpers from YAML schema (util/telemetry/builder/values.yaml) - Required attributes become function parameters - Optional attributes use functional options pattern (WithAttributeName) - Support for both regular and observable instruments - Preserve original attribute types (bool, int, string) without coercion Implementation: - Add metrics_helpers.go generator in util/telemetry/builder/ - Generate util/telemetry/metrics_helpers.go with all helper methods - Migrate all workflow metrics to use type-safe helpers - Use method values (closures) instead of storing *Metrics pointers - Refactor work_queue.go to use helpers while maintaining k8s interface compatibility Benefits: - Compile-time validation prevents attribute typos and missing attributes - Zero runtime overhead compared to direct API usage - Type safety for attribute values (no string coercion of bools/ints) - Clear API showing which attributes are required vs optional - Eliminates code smell of storing *Metrics pointers in structs Breaking Changes: None (helpers wrap existing API, backward compatible) Low-level API Usage: Low-level instrument API is now confined exclusively to metrics_custom.go for dynamic user-defined metrics that cannot be known at compile time. Files Changed: - util/telemetry/builder/builder.go: Add helper generation logic - util/telemetry/builder/metrics_helpers.go: Helper generator implementation - util/telemetry/builder/values.yaml: Add type field to attributes - util/telemetry/metrics_helpers.go: Generated type-safe helpers (11KB) - workflow/metrics/*: Migrate to use helpers and method values - docs/metrics.md: Regenerate documentation Results: - All metrics use type-safe helpers with compile-time validation - work_queue.go simplified by 81 lines (30% reduction) - No *Metrics pointers stored in structs - All tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Alan Clucas <alan@clucas.org> Co-Authored-By: Alan Clucas <alan@clucas.org> Co-Authored-By: Claude <noreply@anthropic.com>
Joibel
force-pushed
the
claude/optimize-metric-attributes-011CUQBVWgg2ccfmAWp7QbhA
branch
from
f31e259 to
1cdf835
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Follow up to #13943 where I mention we could enforce attributes as documented in values.yaml in the golang side to ensure they lined up.
The discovery of #14976 lead me to believe this was worth doing now.
It has discovered one missing attribute - see the docs change.
Modifications
Vibed with my mate claude from anthropic to get a generator for helper functions for all builtin metrics types.
Then use those throughout the metrics codebase.
Verification
CI will check the e2e tests, but unit tests pass without modification.
Documentation
Fixed docs for one metric. Otherwise no user facing changes