handle infinite loop - #326
Closed
pranavpadmasali wants to merge 43 commits into
Closed
pranavpadmasali wants to merge 43 commits into
pranavpadmasali wants to merge 43 commits into
Conversation
Implements Phase 1 of the telemetry initiative: - common/telemetry package with Provider, Config, and lifecycle management - OTLP exporter (gRPC and HTTP) with periodic push - Prometheus exporter with HTTP scrape endpoint (port pre-bound synchronously so startup failures surface immediately via New()) - OTel Resource construction with service.name, service.instance.id, service.version attributes - configmgr.Listener implementation for SIGHUP config reload awareness - Standard attribute keys (AttrState, AttrPriority, AttrDirection, etc.) - Histogram bucket configuration support via HistogramConfig - Unit tests covering disabled/enabled paths, all validation rules, in-memory metric recording, Prometheus endpoint, and occupied-port failure
…Phase 2) - Replace expvar with OTel Int64Counter in workmgr (beesync.work.requests with state/priority attributes) and job manager (beeremote.job.requests, beeremote.work.requests with status attribute) - Replace expvar Float/String globals in scheduler with OTel observable gauges (beesync.scheduler.completed.work.rate, beesync.scheduler.tokens.allowed.rate) backed by atomic fields - Add metric.Meter parameter to workmgr.NewAndStart and job.NewManager; wire tp.Meter() from main.go in both services - Add telemetry.Config to AppConfig for beegfs-sync and beegfs-remote with ValidateConfig, GetTelemetryConfig, and compile-time Configurer check - Add telemetry pflags (enabled, service-name, otlp.*, prometheus.*) in both main.go files; default Prometheus ports 9090 (sync) and 9091 (remote) - Add commented telemetry config sections to both build TOML files - Add normalizedPriority helper to preserve legacy priority-0 → 1 mapping so OTel attributes match the prior expvar priorityIdMap behavior - Add nil guard in scheduler.WithMeter to prevent panic on nil interface - Update workmgr and job manager tests to pass noop.NewMeterProvider().Meter
Previously, unexpected errors from srv.Serve were assigned to _ and dropped. Any mid-run server failure (e.g. listener torn out from under the server) would go completely unobserved. Now they are logged via the global zap logger.
- Rewrite TestWithOptions to use an enabled Prometheus provider and
verify resource attributes (service.instance.id, service.version)
appear correctly in target_info labels
- Add body content assertion to TestPrometheusEndpoint
- Add TestShutdownEnabled, TestUpdateConfiguration,
TestHistogramBucketBoundaries, TestOTLPReaderConstruction
- Remove TestMetricRecording which tested OTel SDK behavior, not this
package
Verify beeremote.job.requests, beeremote.work.requests, beeremote.job.terminal, and beeremote.job.duration metrics are correctly recorded on job submission and completion.
Verify beesync.work.requests counter is recorded with correct state attributes when work is submitted and queued.
Verify beesync.scheduler.completed.work.rate and beesync.scheduler.tokens.allowed.rate gauges are observable and report values set in the scheduler atomics.
Embed a telemetry.Provider in Logger so components receive a single observability object instead of separate logger and telemetry parameters. Logger owns the telemetry lifecycle (init + shutdown) and forwards config reloads to the Provider when the AppConfig supports it. Changes: - Add Telemetry field to Logger, always initialized (noop when unconfigured) - Extend New() with optional *telemetry.Config parameter - Add With() that shadows zap.With to preserve Telemetry and atomic level - Add Shutdown() to flush telemetry and sync logger in one call - UpdateConfiguration() forwards config reloads to telemetry Provider - Update beegfs-remote, beegfs-sync, beegfs-watch to use *logger.Logger - Add GetLoggingConfig() to remote and sync AppConfigs - Simplify ctl.InitLoggerFromExternal to accept *logger.Logger - Unify logger+telemetry initialization in all main.go files
Replace scattered recordJobTerminal() call sites with a single defer-based guard in updateJobState() and UpdateWork(). Add isTerminalState() and terminalStateString() helpers to unify terminal state classification. recordJobTerminal() now emits a structured log entry (Warn for failed/unknown, Info for cancelled, Debug for completed/offloaded) in addition to recording metrics. The log includes state, rst_id, job_id, path, and message fields. Also fixes a pre-existing flaky TestManageErrorHandling that relied on a 2s sleep; replaced with require.Eventually (10s/100ms) to accommodate the ~1s mock worker connection setup time.
Add LogsConfig struct and wire it into the telemetry Provider so that services can export logs via OTLP (gRPC or HTTP) independently of whether metrics telemetry is enabled. A service can now run with telemetry.enabled=false (no metrics) but telemetry.logs.enabled=true to ship structured logs to a collector. - ValidateConfig restructured: metrics validation guarded by Enabled, log validation always runs independently - New() builds the OTel resource when either signal is enabled; metrics and log providers are initialized in separate blocks - LogProvider() accessor added for use by the logger otelzap bridge - Shutdown() flushes both providers; UpdateConfiguration() warns only when a live provider exists (no spurious restart warnings) - 4 new telemetry.logs.* pflags added to beegfs-remote and beegfs-sync - [telemetry.logs] commented section added to both TOML config templates
When LogsConfig.Enabled is true, logger.New() tees otelzap alongside the existing console/file/syslog sink so log entries are exported via OTLP. zapcore.NewIncreaseLevelCore gates the otelzap core on the same atomic level as the console sink so OTLP never receives records below the configured threshold. - Add LogsConfig validation: ServiceName required when any signal enabled - Add protocolGRPC/protocolHTTP constants across telemetry package - Refactor logger.New() to initialize telemetry once before the if/else - Replace hand-rolled leveledCore with zapcore.NewIncreaseLevelCore - Revert buildResource to accept []Option (resolveOptions stays internal)
Replace the non-standard `insecure` field in OTLPConfig and LogsConfig with the three-field TLS pattern used by every other client connection in this codebase: `tls-cert-file`, `tls-disable-verification`, `tls-disable`. Adds buildClientTLSConfig following the same pattern as beegrpc.NewClientConn: start from the system cert pool and optionally extend with a custom CA cert. Updates flag registrations in beegfs-remote and beegfs-sync, and updates the shipped TOML config files to drop the old insecure field.
… endpoint Adds one-way server-side TLS to the Prometheus scrape endpoint, making it consistent with the beegfs-remote/sync gRPC servers. Operators configure tls-cert-file and tls-key-file; the Prometheus scraper uses tls_config.ca_file to verify the server cert. Also adds listen-address to control the bind interface.
Adds three new fields to both OTLPConfig and LogsConfig: - url-path (HTTP only): allows overriding the OTLP HTTP path, e.g. "/otlp/v1/logs" for direct Loki ingestion without a collector - compression: "gzip" or "none"; gzip is recommended and now the default for non-local endpoints - timeout: per-export deadline; validated to be >= 1s if set All four exporters (metrics gRPC/HTTP, logs gRPC/HTTP) wire the new options. gRPC branches warn if url-path is set since it is ignored. Pflags are registered in both beegfs-remote and beegfs-sync with the same defaults. Config file examples are updated with the new fields prefilled, and the [telemetry] block is now live (disabled by default) so operators don't need to uncomment it to get started.
prometheus.NewGoCollector and prometheus.NewProcessCollector are deprecated since client_golang v0.9.0; use collectors.NewGoCollector and collectors.NewProcessCollector from the collectors subpackage.
…rovider accessors The Telemetry field was exported unnecessarily, leaking the concrete *telemetry.Provider type into callers. Make it private and add two typed accessor methods so components can obtain meters and the log provider without depending on the Provider type directly. Update all callers to use log.Meter() instead of log.Telemetry.Meter().
…cleanup contexts Validate that tls-disable and tls-disable-verification are not set together for both OTLP metrics and logs — tls-disable-verification only applies when TLS is active. Replace context.Background() in error-path cleanup with a 5s bounded context so a stalled provider shutdown cannot block New() indefinitely before signal handling is up.
…own timeout Add pflags for telemetry.prometheus.tls-cert-file, tls-key-file, tls-disable and listen-address to both service binaries. Replace the bare context.Background() shutdown defer with a 10s bounded context so the OTel SDK has time to flush the final metrics window on exit.
…tests Production code: - Consolidate dual error-path cleanup in New() into a single cleanupFns slice drained by a shared cleanup() closure; adds logProvider to the slice for safety against future code additions after its construction - Call ValidateConfig() in UpdateConfiguration before storing new config so invalid SIGHUP reloads are rejected rather than silently accepted - Split the conflated "telemetry or logs" service-name error into two distinct messages so operators know which signal requires the name - Add reflect.DeepEqual caveat comment (unexported fields in embedded third-party types would silently break the comparison) - Add context.Background() explanation to buildOTLPReader and buildLogExporter function doc comments (lazy connect; one place not four) - Add "keep in sync" cross-reference comment between buildOTLPReader and buildLogExporter which share identical control flow Tests: - Extract assertLogContains helper to replace five copy-paste log-scan loops - Extend observeGlobalLogger to accept a zapcore.Level parameter - Add TestOTLPDefaultTLSConstruction: nil TLS config path smoke test - Add TestHistogramBucketBoundaries/BytesBoundaries: custom byte bucket boundaries appear in Prometheus output - Add TestOTLPReaderGRPCURLPathWarning: url-path warning for gRPC protocol - Add TestLogsGRPCURLPathWarning: same for log export - Add TestPrometheusServerTLSDisableLog: info log when TLS explicitly disabled - Remove TestOTLPReaderConstruction and TestLogsExporterNewFields: both only asserted NoError on lazy construction with no behavioral coverage - Fix TestPrometheusEndpoint: remove redundant assert.NotEmpty - Fix TestPrometheusEndpointTLS: use t.Context(), remove nolint comment, clarify platform-specific plain-HTTP assertion - Improve TestUpdateConfiguration/toggling subtest name and comment clarity
- Move attribute keys out of common/telemetry: service-specific keys (attrStatus, attrRSTID, attrPriority) are now unexported vars in their respective packages; attrState is duplicated locally in each service; dead-code keys (AttrDirection, AttrOperation, AttrErrorType, AttrRSTType) removed per YAGNI - Add DeferredShutdown helper to logger, replace duplicated shutdown closures in both beegfs-remote and beegfs-sync main.go - Refactor logger.New() developer branch to use early return - Fix beegfs-remote.toml: correct Section 1 ordering (1.1→1.2→1.3→1.4), expand TOC with subsection entries, renumber RST docs from 6→5 - Update With() doc comment to describe behavior rather than implementation
Extract validateOTLPTransport to consolidate the five shared transport field checks (protocol, endpoint, compression, timeout, TLS mutual- exclusivity) that were duplicated between OTLPConfig and LogsConfig validation. Add test cases for the TLS mutual-exclusivity rejection path through both call sites.
Remove comments that were redundant with code, described usage/call-sites,
or used temporal phrasing ("currently only", "For v1").
… and server invariant - scheduler: store OTel RegisterCallback Registration handle and call Unregister() in close() to prevent the callback from firing after shutdown and holding a reference to the Scheduler - logger: propagate telemetry UpdateConfiguration errors to the caller instead of logging a warning and returning nil, so operators see config validation failures on SIGHUP - exporters: assign p.promServer only after TLS setup succeeds, preserving the invariant that promServer is non-nil iff the server goroutine is running - add tests covering all three fix paths
…l metrics to workermgr Replaces the noisy single-state beeremote.work.requests counter with two properly designed instruments on workermgr.Manager: - beeremote.work.active (Int64UpDownCounter): tracks WRs currently in non-terminal states, labeled by state and rst.id - beeremote.work.terminal (Int64Counter): counts WRs that reached a terminal state (completed, cancelled, failed, unknown), labeled by terminal state and rst.id recordInitialWork() handles first entry into the metric system (SubmitJob). recordWorkTransition() handles all subsequent state changes (UpdateJob), with a guard on isTerminalWorkState(oldState) to prevent double-counting. WorkActive and WorkTerminal are exported so job.Manager can record transitions in UpdateWork() (wired in the next commit). RSTID added to JobUpdate to carry the RST ID through the cascade cancellation path in SubmitJob.
…move beeremote.work.requests Remove beeremote.work.requests (single call site, hardcoded status="created", pure noise). Wire WorkActive/WorkTerminal from workermgr.Manager into UpdateWork() — records per-WR SCHEDULED→terminal transition before the early-return loop so every WR arrival is counted regardless of job outcome. Also: export workStateString→WorkStateString for cross-package use, add RSTID to UpdateJob call in updateJobState, and add C1–C6 (per-WR terminal states) + D1–D2 (RST isolation + balance) test coverage.
service-name is an identity field, not a transport option. Moving it out of [telemetry] lets operators see it alongside mount-point and makes it clear it applies to all telemetry signals (metrics and logs). telemetry.Config.ServiceName → AppConfig.ServiceName in both beegfs-remote and beegfs-sync. telemetry.New now accepts WithServiceName() alongside the existing WithVersion/WithInstanceID options.
Each sub-exporter (OTLP, Prometheus, logs) already has its own Enabled flag. The top-level Config.Enabled was redundant and required operators to set two flags to enable any exporter. Remove it entirely so a single sub-exporter Enabled flag is sufficient. ValidateConfig now validates each sub-config unconditionally based on its own Enabled flag. New() determines noop vs real provider from cfg.OTLP.Enabled || cfg.Prometheus.Enabled rather than cfg.Enabled. Remove the telemetry.enabled pflag from both beegfs-remote and beegfs-sync. Remove the stale enabled field and doc comments from both default TOML configs.
…ngle listen-address Combining a separate address and port into a single host:port string matches how every other service in the ecosystem configures listen addresses and removes one config field operators must set. Remove Port from PrometheusConfig. ListenAddress now takes a full host:port value (e.g. "0.0.0.0:9090"). ValidateConfig rejects empty addresses and validates format via net.ResolveTCPAddr. Default flags for both beegfs-remote (9091) and beegfs-sync (9090) are updated. Breaking change: operator configs with telemetry.prometheus.port set will fail startup — UnmarshalExact rejects the now-unknown key.
…0 and there are no negative jobs in any state.
Prevents beeremote.work.active going negative on sync node restart.
WorkActive is now monotonic (only increments on entering a non-terminal
state, never decrements). Current occupancy of state S is computable as:
increase(work.active{state=S}) minus increases for all successor states
of S, minus beeremote.work.terminal for WRs whose last non-terminal
state was S.
Two bugs fixed: 1. telemetry.New() was called before the zap logger was built, so zap.L() calls inside telemetry/exporters.go hit the noop global and dropped warnings (e.g. "TLS verification disabled"). 2. Prometheus could start before logger validation completed. A bad log level or unreachable log file would return an error after telemetry.New() had already started the HTTP server, leaking it. Fix: build the zap logger first, call zap.ReplaceGlobals() so telemetry warnings route to the service logger, then call initTelemetry(). On any error after ReplaceGlobals, undo() restores the noop global. The dev and production paths both follow this order; developer mode returns early after initTelemetry so no otelzap tee is needed there.
DeferredShutdown was a thin wrapper that stacked a redundant timeout on top of the OTel exporter's own per-export timeout. Official OTel Go SDK examples use context.Background() for shutdown; the exporter's configured timeout (otlp.timeout) already bounds how long the final flush can take. Replace all call sites with an inline defer that calls Shutdown directly with context.Background() and logs any errors via stdlib log.
Fixes 4 vulnerabilities reported by govulncheck: - GO-2026-4982: XSS in html/template (https://pkg.go.dev/vuln/GO-2026-4982) - GO-2026-4980: XSS in html/template (https://pkg.go.dev/vuln/GO-2026-4980) - GO-2026-4971: net panic on Windows with NUL byte (https://pkg.go.dev/vuln/GO-2026-4971) - GO-2026-4918: HTTP/2 infinite loop in x/net and net/http (https://pkg.go.dev/vuln/GO-2026-4918) All four are fixed in go1.26.3. GO-2026-4918 also requires golang.org/x/net >= v0.53.0. Transitive x/ dependencies (sys, term, mod, text, tools, telemetry) bumped alongside.
pranavpadmasali
force-pushed
the
pranav/telemetry
branch
3 times, most recently
from
June 3, 2026 12:22
27aff9e to
77ad1f9
Compare
Comment on lines
+206
to
+209
| } else { | ||
| // connectLoop returns false when context is cancelled or an unrecoverable | ||
| // error occurs. In both cases stop retrying. | ||
| done = true |
Member
There was a problem hiding this comment.
question: what happens now if a Sync node is down? Do we try to reconnect to it elsewhere, just perhaps with a backoff so we don't spam the logs?
If that is the case this PR is fine, but if we stop trying to reconnect altogether that is not what we want.
Contributor
There was a problem hiding this comment.
If the node is down then BeeSyncNode.connect() will return a client heartbeat error with retry=true so we would continue to try reconnecting. However, any version/configuration errors return retry=false along with the error which will result in a shutdown.
Contributor
|
As I was considering this PR, I realized that deeper changes are needed. There are actually two issues,
#333 addresses both of these issues |
pranavpadmasali
force-pushed
the
pranav/telemetry
branch
2 times, most recently
from
June 16, 2026 12:24
1138feb to
ffbd2d6
Compare
Contributor
Author
|
Closing in favour of #333 |
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
3 participants
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.
What does this PR do / why do we need it?
Handle()runs outer loop withdoneflag. Inner call toconnectLoop()returns bool — true means reconnect, false means stop. Code only handled true case. In case of false theconnectLoop()is called multiple times thus spamming the logs.Fix: Add else { done = true } so when connectLoop returns false, outer loop exits.
Reproducer
start
journalctl -u beegfs-remote -fon another terminalLogs before fix
Logs after fix
Checklist before merging:
Required for all PRs.
When creating a PR these are items to keep in mind that cannot be checked by GitHub actions:
For more details refer to the Go coding standards and the pull request process.