refactor: fix architecture issues - env layering and healthcheck metrics extraction - #107
refactor: fix architecture issues - env layering and healthcheck metrics extraction#107jongio wants to merge 3 commits into
Conversation
…xecute fileutil: Extract shared atomicWrite() and renameWithRetry() helpers, eliminating duplicated temp-file write/sync/chmod/rename logic between AtomicWriteJSON and AtomicWriteFile. (Fixes #89) httpclient: Decompose monolithic Execute (208 lines) into focused helpers - buildRedirectClient, buildRequest, logVerboseRequest, executeWithRetry, bufferBodyForRetry, resetRequestBody, buildResponse. Add effective*() accessors for config defaults. Group RequestOptions fields by concern. (Fixes #87, #90)
Extract CircuitBreakerManager, RateLimiterManager, and EndpointCache from the monolithic HealthChecker struct. Each manager encapsulates its own mutex and data, improving separation of concerns and testability. - CircuitBreakerManager: manages circuit breaker creation and state queries - RateLimiterManager: manages per-service rate limiters - EndpointCache: manages endpoint URL caching with none-sentinel support (Fixes #88)
Issue #49 - Extract healthcheck metrics into sub-package: - Create healthcheck/metrics/ sub-package with all Prometheus metrics logic - healthcheck/metrics.go becomes thin wrapper delegating to sub-package - Consumers can now import healthcheck types without Prometheus dependency Issue #48 - Remove env -> keyvault layering violation: - Define ResolveOptions, ResolutionWarning, Resolver locally in env package - Duplicate IsKeyVaultReference patterns in env (intentional, breaks cycle) - Create keyvault/envadapter.go to bridge type systems - env package no longer imports keyvault
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Five issues, two are behavioral regressions that'll change runtime behavior silently.
The layering fix (env/keyvault) and metrics extraction are solid. The god-struct decomposition is clean. But the circuit breaker and rate limiter extractions quietly changed semantics, and there's a script that shouldn't be here.
| @@ -0,0 +1,388 @@ | |||
| import subprocess, os, sys, re, time | |||
There was a problem hiding this comment.
This automation script has hardcoded local paths and shouldn't be committed. Delete before merge.
| MaxRequests: 1, | ||
| Timeout: m.timeout, | ||
| ReadyToTrip: func(counts gobreaker.Counts) bool { | ||
| return int(counts.ConsecutiveFailures) >= m.maxFail |
There was a problem hiding this comment.
This changes tripping logic from ratio-based (60% failure rate + minimum request count) to consecutive-failure-based. The original waited for requests >= breakerFailures && failureRatio >= 0.6. This version trips on N consecutive failures regardless of total volume - that's a more aggressive tripping policy. Was this intentional?
Also MaxRequests dropped from 3 to 1, meaning only 1 request is allowed through in half-open state instead of 3.
|
|
||
| // GetOrCreate returns the circuit breaker for the given key, creating one if needed. | ||
| // Returns nil if circuit breaking is disabled. | ||
| func (m *CircuitBreakerManager) GetOrCreate(key string) *gobreaker.CircuitBreaker { |
There was a problem hiding this comment.
The original breaker had an OnStateChange callback that called recordCircuitBreakerState for Prometheus metrics. This extraction drops it, so circuit breaker state transitions won't emit metrics anymore. Need to either pass a callback into newCircuitBreakerManager or wire it back in.
| defer m.mu.Unlock() | ||
| if lim, ok := m.limiters[key]; ok { | ||
| return lim | ||
| } |
There was a problem hiding this comment.
Burst changed from rateLimit*2 to rateLimit. Original used rate.NewLimiter(rate.Limit(c.rateLimit), c.rateLimit*2). This halves burst capacity, meaning concurrent requests get throttled more aggressively. If the original 2x burst was intentional, this is a regression.
| return nil, fmt.Errorf("request failed after %d retries: %w", maxRetries, lastErr) | ||
| return nil, fmt.Errorf("request failed after %d retries: %w", maxRetries, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
bodyBytes parameter is unused here - the _ = bodyBytes is just suppressing the compiler. Either remove the parameter or use it to recreate a bytes.Reader when the existing reader can't seek.
|
Closing - will consolidate with other conflicting PRs into a single clean PR against current main. |
1 participant
Summary
Fixes #48 and #49 - two architecture issues identified in the project review.
Issue #48: env -> keyvault layering violation
The \�nv\ package imported \keyvault\ directly, creating a dependency inversion where a utility-level package depended on a domain-level Azure service package.
Fix:
Issue #49: healthcheck mixed concerns
The healthcheck package bundled Prometheus metrics with core health check types, forcing consumers to pull in Prometheus dependencies even when only needing types.
Fix:
Additional cleanup (supporting commits)
Testing
All 27 packages pass: \go test ./...\ green.