fix: register explicit bucket boundaries for duration histograms - #2255
fix: register explicit bucket boundaries for duration histograms#2255cowsking wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test kpt-config-sync-presubmit |
| @@ -51,6 +51,14 @@ const ( | |||
| InternalErrorsName = "internal_errors_total" | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Thanks for working on the fix, could you check if the fix apply to pkg/kmetrics as well?
There was a problem hiding this comment.
Good catch, thanks! Pushed an update to include pkg/kmetrics: added explicit second-scale bucket bounds for kustomize_build_latency, fixed a pre-existing bug in exec.go where raw nanoseconds were recorded as duration and added unit test coverage.
1a1956e to
535a861
Compare
| if err == nil && sendMetrics { | ||
| RecordKustomizeResourceCount(ctx, resourceCount) | ||
| RecordKustomizeExecutionTime(ctx, float64(executionTime)) | ||
| RecordKustomizeExecutionTime(ctx, executionTime.Seconds()) |
There was a problem hiding this comment.
Record in ms, to align with pkg/kmetrics/metrics.go
There was a problem hiding this comment.
Done. Updated to float64(executionTime.Milliseconds())
| metric.WithDescription("Kustomize build latency"), | ||
| metric.WithUnit("ms"), | ||
| metric.WithDescription("Kustomize build latency in seconds"), | ||
| metric.WithUnit("s"), |
There was a problem hiding this comment.
Keep metric.WithUnit("ms"), set KustomizeBuildLatencyBounds to match its original millisecond exponential buckets. Context go/config-sync-monarch-metrics some metrics are predefined in Monarch, changes in unit might cause error like INVALID_ARGUMENT: The bucket options / unit do not match the metric descriptor
There was a problem hiding this comment.
Done. Kept the "ms" unit and restored the exponential buckets ([0, 10, 20, ..., 10240]) to match the Monarch descriptor.
| DistributionBounds = []float64{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10} | ||
|
|
||
| // LongDistributionBounds defines the bounds for a histogram distribution measuring long durations. | ||
| LongDistributionBounds = []float64{.005, .01, .025, .05, .1, .25, .5, 1, 5, 10, 30, 60, 300, 600, 1200, 1800, 3600, 5400} |
There was a problem hiding this comment.
Could you check whether LongDistributionBounds (parser_duration_seconds, apply_duration_seconds) should start at 1 instead of .005 to match Monarch's pre-registered descriptor ([1, 5, 10, ...]).
There was a problem hiding this comment.
Updated LongDistributionBounds to start at 1 ([1, 5, 10, ..., 5400]) to align
Following the OpenCensus to OpenTelemetry metrics migration, duration
histogram instruments in Config Sync lacked explicit bucket boundaries.
This caused the OpenTelemetry Go SDK to fall back to default
millisecond-scale boundaries {0, 5, 10, 25, ...}, which for second-unit
metrics resulted in all sub-5-second values falling into the (0, 5] bucket
and skewing Prometheus histogram_quantile queries (e.g. static 4.75s).
This change:
- Configures explicit sub-second bucket boundaries for core duration
metrics (APICallDuration, ReconcileDuration, RemediateDuration) and
long duration metrics (ParserDuration, ApplyDuration).
- Configures explicit bucket boundaries for ResourceGroup ReconcileDuration.
- Adds unit tests to verify histogram bucket bounds and sub-second data
point placement.
Fixes b/545631804
535a861 to
3e1eeff
Compare
2 participants
Following the OpenCensus to OpenTelemetry metrics migration, duration histogram instruments in Config Sync lacked explicit bucket boundaries. This caused the OpenTelemetry Go SDK to fall back to default millisecond-scale boundaries {0, 5, 10, 25, ...}, which for second-unit metrics resulted in all sub-5-second values falling into the (0, 5] bucket and skewing Prometheus histogram_quantile queries (e.g. static 4.75s).
This change:
pkg/kmetricskustomize_build_latency: the histogram recorded raw nanosecond values while declaring a millisecond unit, so every recording landed in the overflow (+Inf) bucket regardless of bucket configuration. It now records the duration in seconds (consistent with all other Config Sync duration metrics), declares the unit ass, and registers explicit bucket boundaries. Note this changes the scale of the exported values (previously nanosecond-scale numbers labeled as ms).