Production-readiness improvements for the Aerospike Connection Manager - #77
Open
burstmeman wants to merge 9 commits into
Open
Production-readiness improvements for the Aerospike Connection Manager#77burstmeman wants to merge 9 commits into
burstmeman wants to merge 9 commits into
Conversation
- Replace the hand-written, type-asserted key extraction with a koanf unmarshal into the already toml-tagged AerospikeFlags, bridging the custom flag types (passwords, host[:tls][:port] seeds, certificates, auth, TLS protocols) with a single Set(string)-based decode hook - Declare clusters under [clusters.<name>] so cluster definitions are explicit; read a top-level table as a cluster only when it has a host or socket, so auxiliary tables like [uda] are no longer turned into bogus clusters - Keep legacy top-level cluster tables working but report their names so the daemon logs a startup deprecation notice guiding migration - Cover parsing, discovery, the namespace and deprecation with tests
- Introduce internal/config resolving the operational ("management") settings
from a strict precedence chain — defaults < [management] TOML < ASLD_* env <
CLI flags — so observability can be toggled without rebuilding or editing
files; source merging and struct mapping are delegated to knadh/koanf
- Reserve the [management] table so process-global settings are never parsed
as a cluster, and validate the result (non-empty address, rooted paths, no
path collisions) to fail fast instead of panicking the HTTP mux at runtime
- Cover precedence, validation and every source adapter with unit tests
- Add internal/metrics bundling Go runtime, process and gRPC server collectors behind one registry, giving operators standard visibility into goroutines, memory, CPU and per-method request rate and latency - Tune the gRPC latency histogram for asld's sub-millisecond profile so the buckets resolve real latencies instead of collapsing into Prometheus' 5ms default floor, and pre-register method series so dashboards populate at deploy time rather than after first use - Pull in prometheus/client_golang and the grpc-middleware Prometheus provider; client_golang v1.23 requires the go directive at 1.23 - Cover registry contents, the /metrics handler and interceptors with tests
- Expose per-cluster connection-pool health (open connections, node count, connection attempts/failures, pool-empty/overflow, idle drops, failed tends) so pool exhaustion and node loss are visible before they surface as errors - Implement it as a prometheus.Collector that reads client.Stats() at scrape time rather than polling on a timer: values stay fresh, no extra goroutine, and a cluster whose scrape fails reports up=0 without hiding the others - Keep the package Aerospike-free behind a small StatsProvider interface and cover healthy, failed and multi-cluster scrapes with tests
- Add internal/health serving /livez, /readyz and /healthz with JSON bodies and 200/503 semantics so Kubernetes (and any HTTP probe) can drive restart and load-balancing decisions - Keep liveness free of dependency checks and put Aerospike connectivity on readiness only, so a cluster outage drains traffic from the pod instead of restarting an otherwise-healthy process - Bound each probe with a timeout and cover pass, fail, aggregation and liveness-independence with tests
- Add internal/management serving metrics, liveness, readiness, health and optional pprof on a single admin port, separate from the gRPC data path, so one port covers scraping and probes without touching the data plane - Mount each route only when enabled in the resolved config (pprof stays off unless explicitly turned on), bind synchronously so an address-in-use error fails startup instead of vanishing into a goroutine, and shut down gracefully - Cover routing, disabled endpoints, readiness propagation and lifecycle
- Start the management server (metrics + probes) alongside the per-cluster gRPC servers, register a readiness check and a stats source per cluster so the endpoints reflect real state, and add the gRPC metrics interceptor with pre-registered methods - Replace the goroutine log.Fatalln calls with a single signal-driven path: setup failures abort with cleanup, and SIGINT/SIGTERM trigger GracefulStop plus management drain so in-flight RPCs and probes finish cleanly - Switch to structured slog logging with cluster context and surface client warm-up and panic-recovery details instead of discarding them
- Point the Dockerfile CMD and the packaged systemd unit at -config-file, the flag the binary actually defines; they passed --config, which the binary rejects, so the container and the installed service could never start - Drop the reference to the absent docker-entrypoint.sh and ship a working default asld.toml so the image builds and runs without a missing script - Build the image on Go 1.23 to match the module's go directive - Correct the doubled https:// scheme in the packaged unit's Documentation URL
- Add OBSERVABILITY.md: configuration precedence, a full settings reference (TOML/env/flag/default), the endpoint and metrics catalogues, Kubernetes probe and scrape examples, and security guidance - Move the example configs to the [clusters.<name>] format, document the [management] section and the legacy-cluster deprecation, and point the README at the new layout and the observability guide
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
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.
Context & motivation
We're in the middle of migrating our platform from PHP 7 to PHP 8, and the
Aerospike Connection Manager (ACM) + the PHP client sit on the critical path of
that move. As part of it we need ACM to be operable as a first-class production
service — today it has no metrics, no health/readiness endpoints, no graceful
shutdown, and a couple of startup papercuts (the Docker image and the packaged
systemd unit reference a flag the binary doesn't define, so they can't actually
start).
We've standardized our stack on Aerospike and this client, which means ACM's
production-readiness directly gates ours — so naturally we'd much rather build
on top of and contribute back to the upstream project than carry a private
fork indefinitely. That's the spirit of this contribution: get the component to
a state where it's safe to run in our (and hopefully everyone's) clusters.
To unblock our testing right now we've taken a patching approach against
ACM, but we'd really like these changes to live in open source rather than as
out-of-tree patches.
What's in here
A set of opt-in, fully configurable operational features, plus some
foundational cleanup:
knadh/koanf—replaces ~190 lines of hand-written, type-asserted key extraction with an
unmarshal into the already-
toml-tagged struct (one decode hook bridges thecustom flag types). Behaviour is pinned by characterization tests.
[clusters.<name>]config namespace — explicit cluster declaration;legacy top-level tables still work but log a startup deprecation notice. This
also fixes a latent quirk where non-cluster tables (e.g.
[uda]) weresilently parsed as bogus localhost clusters.
defaults < [management] TOML < ASLD_* env < CLI flags, with a clear,documented precedence.
tuned for ACM's sub-millisecond profile), and per-cluster Aerospike
connection-pool metrics exposed via a scrape-time collector.
/livez,/readyz(gated on Aerospikeconnectivity),
/healthz, plus opt-inpprof, all on a single, configurableadmin port.
GracefulStop+drain, and removal of
log.Fatallncalls from goroutines.OBSERVABILITY.md).Everything is opt-in and backward compatible; each change is covered by tests,
and every commit builds and passes the suite independently.
Why one PR (for now)
I've intentionally bundled this into a single PR with focused, self-contained
commits so you can see the overall direction and shape of what we'd like
to do, end to end, rather than reviewing it piecemeal without context.
I fully agree this should ultimately be merged as several separate PRs —
and I'm happy to split it that way. Right now the goal is a design-level
conversation: if you have comments or a different preferred shape, I'd be glad
to discuss and rework the changes to match whatever we agree on before anything
gets merged.
Roadmap
This is only the beginning — we also have planned work on the Rust extension
and further ACM improvements. We're committed to doing this in the open and
upstreaming it, so we'd really appreciate your guidance and support in shaping
these contributions.
Thanks for taking a look — looking forward to your feedback!