From c4df82e8c6dece6634412f28ef7ee592b5bf3eeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Germain=20Carr=C3=A9?= Date: Sun, 30 Aug 2026 00:42:38 +0200 Subject: [PATCH 1/6] Say which setting is refusing the change The screen printed "set AllowWriteActions in [Http]" whatever the reason, so a push client that had simply not opted in sent its operator editing the wrong file on the wrong machine. The api names the one that applies now. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 12 +++++ src/public/src/js/views/Disabled.vue | 9 +++- src/public/src/js/views/Host.vue | 12 +++++ src/wigo/http.go | 16 +++++- src/wigo/openapi.yaml | 8 +++ src/wigo/push_commands_test.go | 73 ++++++++++++++++++++++++++++ src/wigo/remote_control.go | 27 ++++++++-- 7 files changed, 150 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 22690a4..9e0d3c9 100644 --- a/README.md +++ b/README.md @@ -472,6 +472,18 @@ A recheck asked for by hand takes whichever is shorter, the configured timeout o The two `POST` endpoints return **403** unless `AllowWriteActions` is set in the `[Http]` section. They act on the probes directory directly, so the change takes effect on the next cycle without a restart, and it survives one. +**A host that pushes is governed by `AllowRemoteControl`, not by that.** Three different things can make a host read only from here, and each is fixed in a different file: + +| what refuses | where to fix it | +|---|---| +| the caller's role | sign in, or present an operator token | +| this host's own writes | `AllowWriteActions` in `[Http]`, on that host | +| a pushing client that never opted in | `AllowRemoteControl` in `[PushClient]`, on the client | + +The API says which, in `ReadOnlyReason` on the schedule, because only it can tell them apart — the interface sees one boolean. It used to print "set AllowWriteActions in `[Http]`" for all three, which sent anyone with a push client editing the wrong file on the wrong machine. + +A client reports `AllowRemoteControl` on **every** push, so opening it takes effect on the next one — ten seconds by default, with nothing to do on the master. + A probe must be installed to be acted upon: a name that exists nowhere under `probes/` is refused. **Disabling never destroys anything.** A schedule is usually a symlink into `examples/`, where the probe itself stays, so the symlink is simply removed. But an administrator may have dropped a script straight into an interval directory, or linked to one outside the probes tree — deleting that would be the only copy gone, and the probe would not even be listed any more, so there would be no way to turn it back on. In that case the entry is moved into `examples/` instead. Either way the probe ends up installed and unscheduled, which is what disabled means. diff --git a/src/public/src/js/views/Disabled.vue b/src/public/src/js/views/Disabled.vue index aa5343d..0ddabf0 100644 --- a/src/public/src/js/views/Disabled.vue +++ b/src/public/src/js/views/Disabled.vue @@ -134,7 +134,10 @@ :probe-name="probe.Name" :schedule="probe" :editable="host.WriteActionsAllowed" - :read-only-reason="`Read only: set AllowWriteActions in the [Http] section of the configuration file on ${host.Name}`" + :read-only-reason=" + host.ReadOnlyReason || + `Read only: set AllowWriteActions in the [Http] section of the configuration file on ${host.Name}` + " @changed=" (updated) => onChanged(host.Name, probe.Name, updated) " @@ -209,6 +212,10 @@ const hostsWithDisabled = computed(() => return { Name: schedule.Hostname, WriteActionsAllowed: !!schedule.WriteActionsAllowed, + // Dite par le serveur : lui seul distingue le rôle de l'appelant, un + // host qui refuse les écritures, et un client qui pousse sans avoir + // accepté d'être piloté -- trois fichiers différents à éditer. + ReadOnlyReason: schedule.ReadOnlyReason || "", DisabledCount: disabled.length, // Triées par ancienneté : celle qui est éteinte depuis huit mois est // celle qu'il faut voir, et elle est en haut. Les sondes que personne diff --git a/src/public/src/js/views/Host.vue b/src/public/src/js/views/Host.vue index af52903..9787f5f 100644 --- a/src/public/src/js/views/Host.vue +++ b/src/public/src/js/views/Host.vue @@ -285,8 +285,20 @@ const canEditSchedule = computed( () => hasSchedule.value && !!schedule.value.WriteActionsAllowed, ); +/** + * Pourquoi c'est en lecture seule, dit par le serveur. + * + * Trois causes possibles et trois fichiers différents : le rôle de l'appelant, + * ce host qui refuse les écritures, et un client qui pousse sans avoir accepté + * d'être piloté. L'interface ne peut pas les distinguer -- elle ne voit qu'un + * booléen -- et la phrase écrite en dur ici envoyait éditer `[Http]` même quand + * le verrou était `AllowRemoteControl` sur la machine d'en face. + * + * Le repli sert pour un master trop ancien pour envoyer la raison. + */ const readOnlyReason = computed( () => + schedule.value?.ReadOnlyReason || `Read only: set AllowWriteActions in the [Http] section of the configuration file on ${hostName.value}`, ); diff --git a/src/wigo/http.go b/src/wigo/http.go index afd6a5d..c548953 100644 --- a/src/wigo/http.go +++ b/src/wigo/http.go @@ -414,7 +414,18 @@ func HttpAuthorityRevokeHandler(w http.ResponseWriter, r *http.Request) (int, st type ProbesSchedule struct { Hostname string WriteActionsAllowed bool - Probes []ProbeLocation + + // Why not, when it is false. Said here because only this side can tell the + // three cases apart : the caller's role, this host refusing writes, and a + // pushing client that has not opted into being driven -- and each is fixed + // in a different file. A screen that names the wrong one sends somebody + // editing a setting that was never the problem. + // + // Empty when writes are allowed, and empty from an older wigo, which is why + // the interface keeps a fallback sentence. + ReadOnlyReason string `json:",omitempty"` + + Probes []ProbeLocation // Probes that ran, exited with the special code 13 and asked not to be run // again -- usually because there is nothing for them to check on this host, @@ -440,11 +451,12 @@ func HttpProbesHandler(w http.ResponseWriter, r *http.Request) (int, string) { // The caller's role counts as much as the host's flag : offering a control // that always answers 403 is worse than not offering it. - _, _, mayWrite := httpWriteActionsAllowed(r) + _, refusal, mayWrite := httpWriteActionsAllowed(r) schedule := ProbesSchedule{ Hostname: GetLocalWigo().GetHostname(), WriteActionsAllowed: mayWrite, + ReadOnlyReason: refusal, Probes: locations, SkippedProbes: GetLocalWigo().GetDisabledProbes(), DisableRecords: ProbeDisableRecords(), diff --git a/src/wigo/openapi.yaml b/src/wigo/openapi.yaml index 5b37221..5c155bd 100644 --- a/src/wigo/openapi.yaml +++ b/src/wigo/openapi.yaml @@ -923,6 +923,14 @@ components: WriteActionsAllowed: type: boolean description: Whether this caller could change it, so a client need not try + ReadOnlyReason: + type: string + description: > + Why not, when WriteActionsAllowed is false. Three things can refuse + a change and each is fixed in a different file : the caller's role, + this host's own AllowWriteActions, and a pushing client that has not + set AllowRemoteControl. Absent when the change would be allowed, and + absent from a wigo older than 1.0.1. Probes: type: array items: diff --git a/src/wigo/push_commands_test.go b/src/wigo/push_commands_test.go index a6adf3c..b048ec2 100644 --- a/src/wigo/push_commands_test.go +++ b/src/wigo/push_commands_test.go @@ -1,6 +1,8 @@ package wigo import ( + "encoding/json" + "strings" "testing" ) @@ -186,3 +188,74 @@ func TestApplyProbeCommandValidatesItsInput(t *testing.T) { t.Errorf("The probe has not been moved") } } + +// Three things can make a host read only, and each is fixed in a different +// file. A screen that names the wrong one sends somebody editing a setting that +// was never the problem -- which is what a hardcoded "set AllowWriteActions in +// [Http]" did to every push client that had simply not opted in. +func TestTheReasonForBeingReadOnlyNamesTheRightSetting(t *testing.T) { + setupTestWigo(t, "databases") + LocalWigo.config.Http.AllowWriteActions = true + + // A client that pushes and has not opted into being driven + SetClientAcceptsRemoteControl("uuid-shy", false) + if ClientAcceptsRemoteControl("uuid-shy") { + t.Fatalf("Expected the client to be refusing") + } + + // And one that has + SetClientAcceptsRemoteControl("uuid-open", true) + if !ClientAcceptsRemoteControl("uuid-open") { + t.Fatalf("Expected the client to accept") + } + + // What this host says about its own writes, which is the other reason and + // has to keep naming [Http]. + _, refusal, mayWrite := httpWriteActionsAllowed(testRequest(t)) + if !mayWrite { + t.Fatalf("Writes are on and the caller is an operator, got %q", refusal) + } + + LocalWigo.config.Http.AllowWriteActions = false + _, refusal, mayWrite = httpWriteActionsAllowed(testRequest(t)) + if mayWrite { + t.Fatalf("Expected writes to be refused") + } + if !strings.Contains(refusal, "AllowWriteActions") || !strings.Contains(refusal, "[Http]") { + t.Errorf("Got %q, expected it to name the setting and its section", refusal) + } +} + +// A client that closes its door again must not keep the orders queued while it +// was open : they would be applied the day it opens for another reason. +func TestTheReasonTravelsWithTheSchedule(t *testing.T) { + setupTestWigo(t, "databases") + LocalWigo.config.Http.AllowWriteActions = false + + _, refusal, _ := httpWriteActionsAllowed(testRequest(t)) + + schedule := ProbesSchedule{ + Hostname: "db1", + WriteActionsAllowed: false, + ReadOnlyReason: refusal, + } + + encoded, err := json.Marshal(schedule) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if !strings.Contains(string(encoded), "ReadOnlyReason") { + t.Errorf("The reason has to reach the interface : %s", encoded) + } + + // And it is left out entirely when writes are allowed, so an older wigo + // answering without it is not mistaken for one that refused silently. + allowed := ProbesSchedule{Hostname: "db1", WriteActionsAllowed: true} + encoded, err = json.Marshal(allowed) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if strings.Contains(string(encoded), "ReadOnlyReason") { + t.Errorf("Got %s, expected no reason when writes are allowed", encoded) + } +} diff --git a/src/wigo/remote_control.go b/src/wigo/remote_control.go index 46f3544..9c830a0 100644 --- a/src/wigo/remote_control.go +++ b/src/wigo/remote_control.go @@ -237,8 +237,10 @@ func HttpHostScheduleHandler(w http.ResponseWriter, r *http.Request) (int, strin // The remote answered what the master may do there, because the master is // who authenticated to it. Whoever is asking here may be allowed less, and // an interface offering a control that answers 403 is worse than none. - if status == 200 && !callerMayWrite(r) { - return status, withWriteActionsOff(body) + if status == 200 { + if _, refusal, mayWrite := httpWriteActionsAllowed(r); !mayWrite { + return status, withWriteActionsOff(body, refusal) + } } return status, body @@ -246,13 +248,17 @@ func HttpHostScheduleHandler(w http.ResponseWriter, r *http.Request) (int, strin // withWriteActionsOff turns the flag down in an answer relayed from a remote, // leaving the rest of it untouched. -func withWriteActionsOff(body string) string { +// +// The reason is this master's, not the remote's : the remote answered that it +// would accept the change, and it is the caller's standing here that refuses it. +func withWriteActionsOff(body string, reason string) string { var schedule ProbesSchedule if err := json.Unmarshal([]byte(body), &schedule); err != nil { return body } schedule.WriteActionsAllowed = false + schedule.ReadOnlyReason = reason rewritten, err := json.Marshal(schedule) if err != nil { @@ -289,9 +295,22 @@ func pushClientSchedule(r *http.Request, hostname string) (int, string, bool) { // What decides whether this host may be changed from here is its own // AllowRemoteControl, not the AllowWriteActions of its local API. + accepted := ClientAcceptsRemoteControl(remote.Uuid) + _, refusal, mayWrite := httpWriteActionsAllowed(r) + + // The client's own refusal is named first : it is the one somebody hits + // after having already opened everything on the master, and pointing them + // at [Http] there would send them editing a file that is not the problem. + if !accepted { + refusal = fmt.Sprintf("%s pushes to this host and has not opted into being driven. "+ + "Set AllowRemoteControl in the [PushClient] section of its own configuration, "+ + "then restart it -- it says so again on its next push.", hostname) + } + schedule := ProbesSchedule{ Hostname: hostname, - WriteActionsAllowed: ClientAcceptsRemoteControl(remote.Uuid) && callerMayWrite(r), + WriteActionsAllowed: accepted && mayWrite, + ReadOnlyReason: refusal, Probes: locations, SkippedProbes: skipped, DisableRecords: disabled, From ff3ba42f83cf0fa2aad71af91b071922cf91f580 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Germain=20Carr=C3=A9?= Date: Sun, 30 Aug 2026 00:54:38 +0200 Subject: [PATCH 2/6] Keep the history of the hosts that push A pushing host cannot be asked anything, so those had no graphs at all and the screen answered a 501 saying why. Their measurements arrive with every push and were dropped ; they are kept now, under the name they came from. Only newer ones : a client pushes every ten seconds for a probe that runs every minute, and writing each arrival stored one measurement six times. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 6 +- src/wigo/global.go | 7 ++ src/wigo/history.go | 191 +++++++++++++++++++++++++++++++++---- src/wigo/history_test.go | 198 +++++++++++++++++++++++++++++++++++++++ src/wigo/openapi.yaml | 7 +- src/wigo/push_server.go | 5 + 6 files changed, 393 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 9e0d3c9..9f6adb8 100644 --- a/README.md +++ b/README.md @@ -586,7 +586,11 @@ It is now written to the SQLite that is already there, bounded by `MetricsRetent Points are **bucketed**: a week at one point a minute is ten thousand points per series, which no browser should be asked to draw. Each bucket carries its average *and* the range it covers, so the spike that woke somebody up is still visible after being averaged. -**Each wigo keeps its own history, and only its own.** A master reads a remote's through that remote's API, the same way it reads its schedule — storing the fleet's series on the master as well would write everything twice and make its database grow with the size of the fleet, which is the thing that pushes people towards a separate stack. A host that pushes rather than being polled cannot be asked, and says so. +**A wigo keeps its own history**, and a master reads a *polled* remote's through that remote's API, the same way it reads its schedule. Storing the whole fleet's series on the master would write everything twice and make its database grow with the size of the fleet, which is the thing that pushes people towards a separate stack. + +**With one exception**, and it is not a softening of that rule but the only place it cannot hold: a host that **pushes** cannot be asked anything — it sits behind a NAT. Its measurements arrive with every push and used to be dropped, so those hosts had no graphs at all and the screen answered a 501 explaining why. They are kept now, under the name of the host they came from. The growth is bounded by the number of pushing clients rather than by the fleet, and those clients are precisely the ones with no other way of being read. + +A client pushes far more often than its probes run — every ten seconds against every minute is a normal pairing — and every push carries the same result again. Only a measurement newer than the last one seen is written, so one probe run is one row rather than six. Without that, the master's database would grow at the push rate for a probe that answered once. Nothing about the monitoring depends on this table: losing it loses history and nothing else. diff --git a/src/wigo/global.go b/src/wigo/global.go index 7223839..192a832 100644 --- a/src/wigo/global.go +++ b/src/wigo/global.go @@ -234,6 +234,13 @@ Options: log.Fatalf("Fail to create table in sqlite database : %s\n", err) } + // A database written before metrics carried a host has to gain the column, + // and its rows have to be attributed to this host : nothing else could have + // been recorded then. + if err = migrateMetricsHost(LocalWigo.sqlLiteConn, LocalWigo.GetHostname()); err != nil { + log.Fatalf("Fail to migrate the metrics table : %s\n", err) + } + _, err = LocalWigo.sqlLiteConn.Exec(createStatusChangesTable) if err != nil { log.Fatalf("Fail to create table in sqlite database : %s\n", err) diff --git a/src/wigo/history.go b/src/wigo/history.go index 99b1114..6aebfd6 100644 --- a/src/wigo/history.go +++ b/src/wigo/history.go @@ -1,6 +1,7 @@ package wigo import ( + "database/sql" "encoding/json" "fmt" "log" @@ -9,6 +10,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" ) @@ -24,25 +26,80 @@ import ( // retention. Nothing about the monitoring depends on it : losing this table // loses history and nothing else. // -// Each wigo keeps its own, and only its own. A master reads a remote's history -// through that remote's api, the same way it reads its schedule. Storing the -// fleet's series on the master as well would write everything twice and make -// the master's database grow with the size of the fleet, which is exactly the -// thing that pushes people towards a separate stack. +// A wigo keeps its own, and a master reads a polled remote's through that +// remote's api, the same way it reads its schedule. Storing the whole fleet's +// series on the master would write everything twice and make its database grow +// with the size of the fleet, which is exactly the thing that pushes people +// towards a separate stack. +// +// With one exception, and it is not a softening of that rule but the only place +// it cannot hold : a host that pushes cannot be asked anything, it sits behind +// a NAT. Its measurements arrive here with every push and used to be dropped, +// so half a fleet had no graphs at all and the screen said so in a 501. Those +// are kept, under the name of the host they came from. The growth is bounded by +// the number of pushing clients rather than by the fleet, and those clients are +// precisely the ones with no other way of being read. const defaultMetricsRetentionDays = 7 +// Rows carry the host they were measured on. Local ones say so too rather than +// leaving it empty : a column that means "here" for some rows and names a host +// for others is one every later query has to remember to special case. const createMetricsTable = ` CREATE TABLE IF NOT EXISTS metrics ( id integer not null primary key, + host text not null default '', probe text not null, tags text not null, value real not null, at int not null ) ; - CREATE INDEX IF NOT EXISTS metrics_lookup ON metrics(probe, tags, at) ; + CREATE INDEX IF NOT EXISTS metrics_lookup ON metrics(host, probe, tags, at) ; ` +// migrateMetricsHost adds the column to a table written before it existed, and +// attributes what is already in there to this host -- which is what it was, +// since nothing else could be recorded then. +func migrateMetricsHost(db *sql.DB, hostname string) error { + rows, err := db.Query(`PRAGMA table_info(metrics);`) + if err != nil { + return err + } + + found := false + for rows.Next() { + var index int + var name, kind string + var notNull, primaryKey int + var fallback interface{} + + if err := rows.Scan(&index, &name, &kind, ¬Null, &fallback, &primaryKey); err != nil { + continue + } + if name == "host" { + found = true + } + } + rows.Close() + + if found { + return nil + } + + if _, err := db.Exec(`ALTER TABLE metrics ADD COLUMN host text not null default '';`); err != nil { + return err + } + + // Everything already there was measured here + if _, err := db.Exec(`UPDATE metrics SET host = ? WHERE host = '';`, hostname); err != nil { + return err + } + + _, err = db.Exec(`CREATE INDEX IF NOT EXISTS metrics_lookup ON metrics(host, probe, tags, at) ;`) + + return err +} + // MetricPoint is one measurement, or one bucket of them. type MetricPoint struct { At int64 @@ -79,7 +136,15 @@ func metricsRetentionDays() int { // the result is already computed, displayed and notified about, and losing a // point of history is not a reason to fail any of that. func RecordProbeMetrics(result *ProbeResult) { - if result == nil || metricsRetentionDays() == 0 { + RecordProbeMetricsOf(GetLocalWigo().GetHostname(), result) +} + +// RecordProbeMetricsOf writes down what a probe measured on a named host. +// +// Used for this host, and for the ones that push to it : their measurements +// arrive with every push and there is nowhere else they could be read from. +func RecordProbeMetricsOf(hostname string, result *ProbeResult) { + if hostname == "" || result == nil || metricsRetentionDays() == 0 { return } if LocalWigo == nil || LocalWigo.sqlLiteConn == nil { @@ -108,7 +173,7 @@ func RecordProbeMetrics(result *ProbeResult) { } statement, err := transaction.Prepare( - `INSERT INTO metrics(probe,tags,value,at) VALUES(?,?,?,?);`) + `INSERT INTO metrics(host,probe,tags,value,at) VALUES(?,?,?,?,?);`) if err != nil { _ = transaction.Rollback() log.Printf("Unable to record the metrics of probe %s : %s", result.Name, err) @@ -127,7 +192,7 @@ func RecordProbeMetrics(result *ProbeResult) { continue } - if _, err := statement.Exec(result.Name, encodeMetricTags(metric["Tags"]), value, at); err != nil { + if _, err := statement.Exec(hostname, result.Name, encodeMetricTags(metric["Tags"]), value, at); err != nil { _ = transaction.Rollback() log.Printf("Unable to record the metrics of probe %s : %s", result.Name, err) return @@ -188,6 +253,12 @@ func decodeMetricTags(encoded string) map[string]string { // eye can read. Each bucket carries its average and the range it covers, so the // spike that woke somebody up is still visible after being averaged. func ProbeMetrics(probe string, since int64, until int64, points int) ([]MetricSeries, error) { + return ProbeMetricsOf(GetLocalWigo().GetHostname(), probe, since, until, points) +} + +// ProbeMetricsOf is the same question asked about a named host, which is how a +// master answers for a client that pushes to it. +func ProbeMetricsOf(hostname string, probe string, since int64, until int64, points int) ([]MetricSeries, error) { if LocalWigo == nil || LocalWigo.sqlLiteConn == nil { return nil, fmt.Errorf("no database to read metrics from") } @@ -214,10 +285,10 @@ func ProbeMetrics(probe string, since int64, until int64, points int) ([]MetricS rows, err := LocalWigo.sqlLiteConn.Query( `SELECT tags, (at / ?) * ? AS bucket, avg(value), min(value), max(value) FROM metrics - WHERE probe = ? AND at >= ? AND at <= ? + WHERE host = ? AND probe = ? AND at >= ? AND at <= ? GROUP BY tags, bucket ORDER BY tags, bucket;`, - bucket, bucket, probe, since, until) + bucket, bucket, hostname, probe, since, until) if err != nil { LocalWigo.sqlLiteLock.Unlock() return nil, err @@ -323,6 +394,12 @@ type ProbeHistory struct { // HttpProbeMetricsHandler answers the history of one probe of this host. func HttpProbeMetricsHandler(w http.ResponseWriter, r *http.Request) (int, string) { + return localMetricsFor(GetLocalWigo().GetHostname(), r) +} + +// localMetricsFor answers from this database, for whichever host the series +// were recorded under : this one, or a client that pushes to it. +func localMetricsFor(hostname string, r *http.Request) (int, string) { probe := r.PathValue("probe") if probe == "" { @@ -334,13 +411,13 @@ func HttpProbeMetricsHandler(w http.ResponseWriter, r *http.Request) (int, strin return 400, err.Error() } - series, err := ProbeMetrics(probe, since, until, points) + series, err := ProbeMetricsOf(hostname, probe, since, until, points) if err != nil { return 400, err.Error() } body, err := json.Marshal(ProbeHistory{ - Hostname: GetLocalWigo().GetHostname(), + Hostname: hostname, Probe: probe, Since: since, Until: until, @@ -373,12 +450,11 @@ func HttpHostProbeMetricsHandler(w http.ResponseWriter, r *http.Request) (int, s return 400, fmt.Sprintf("invalid probe name %q", r.PathValue("probe")) } - // A host that pushes to us cannot be asked : we hold its results, not its - // history, and it is the one keeping that. + // A host that pushes to us cannot be asked, so we answer from what it sent. + // Nothing is relayed for it : this master is where its history lives. if remote := GetLocalWigo().FindRemoteWigoByHostname(hostname); remote != nil { if _, polled := remoteEndpointFor(remote.Uuid); !polled { - return 501, fmt.Sprintf("%s cannot be asked for its history from here : it pushes to this host "+ - "rather than being polled, and each wigo keeps its own measurements.", hostname) + return localMetricsFor(hostname, r) } } @@ -427,3 +503,84 @@ func parseMetricsWindow(r *http.Request) (int64, int64, int, error) { // A cap, because the number of buckets is the number of rows the answer holds // and a caller should not be able to ask for a million of them. const maxMetricsPoints = 2000 + +// The last measurement written down for a host and a probe. +// +// A client pushes far more often than its probes run -- every five seconds +// against every minute is a normal pairing -- and each push carries the same +// result again. Writing it every time stores one measurement a dozen times, +// which is the master's database growing for nothing : the very cost the choice +// of keeping these series here was weighed against. +// +// Kept in memory rather than asked of the database : it is one comparison per +// push against a query per push, and the worst a restart can cost is one +// measurement written twice, which the reading averages away. +var lastPushedMetric = struct { + sync.Mutex + at map[string]int64 +}{at: make(map[string]int64)} + +// alreadyRecorded reports whether this measurement has been seen, and remembers +// it when it has not. +func alreadyRecorded(hostname string, probe string, at int64) bool { + if at == 0 { + return false + } + + lastPushedMetric.Lock() + defer lastPushedMetric.Unlock() + + key := hostname + "\x00" + probe + + if last, known := lastPushedMetric.at[key]; known && at <= last { + return true + } + + lastPushedMetric.at[key] = at + + return false +} + +// ForgetPushedMetrics drops what is remembered about a host, so a client that +// goes away does not hold a key forever. +func ForgetPushedMetrics(hostname string) { + lastPushedMetric.Lock() + defer lastPushedMetric.Unlock() + + for key := range lastPushedMetric.at { + if strings.HasPrefix(key, hostname+"\x00") { + delete(lastPushedMetric.at, key) + } + } +} + +// RecordPushedMetrics keeps what a pushing client just measured. +// +// A client cannot be asked anything -- it sits behind a NAT -- so the only +// moment its measurements can be written down is the moment they arrive. What +// it sends is the same probe results a polled remote would answer with, so +// nothing new travels : this is a place to put them, not a new thing to send. +func RecordPushedMetrics(remote *Wigo) { + if remote == nil || remote.LocalHost == nil { + return + } + + hostname := remote.GetHostname() + if hostname == "" || hostname == GetLocalWigo().GetHostname() { + return + } + + for item := range remote.LocalHost.Probes.IterBuffered() { + probe, ok := item.Val.(*ProbeResult) + if !ok { + continue + } + + // The same result arrives on every push until the probe runs again + if alreadyRecorded(hostname, probe.Name, probe.Timestamp) { + continue + } + + RecordProbeMetricsOf(hostname, probe) + } +} diff --git a/src/wigo/history_test.go b/src/wigo/history_test.go index 6ce3d8b..8870884 100644 --- a/src/wigo/history_test.go +++ b/src/wigo/history_test.go @@ -10,6 +10,12 @@ func setupHistoryTest(t *testing.T) { setupTestWigo(t, "databases") LocalWigo.config.Global.MetricsRetentionDays = 7 + + // What was already pushed is remembered across a whole process, so one test + // would otherwise decide what the next one is allowed to record. + lastPushedMetric.Lock() + lastPushedMetric.at = make(map[string]int64) + lastPushedMetric.Unlock() } func recordAt(t *testing.T, probe string, at int64, metrics ...interface{}) { @@ -238,3 +244,195 @@ func TestProbeMetricsRefusesWhatItCannotAnswer(t *testing.T) { t.Errorf("A window that ends before it starts should be refused") } } + +// A host that pushes cannot be asked anything, so the moment its measurements +// arrive is the only moment they can be written down. Half a fleet had no +// graphs at all before this, and the screen answered a 501 saying why. +func TestAPushingClientsMeasurementsAreKept(t *testing.T) { + setupHistoryTest(t) + + now := time.Now().Unix() + client := newTestRemoteWigo("uuid-push", "behind-nat", "frontend") + + // Oldest first, which is the order a client actually pushes them in + for i := int64(4); i >= 0; i-- { + probe := newTestProbe(client.LocalHost, "load", 100) + probe.Timestamp = now - i*60 + probe.Metrics = []interface{}{metric(float64(i), map[string]interface{}{"metric": "load5"})} + client.LocalHost.Probes.Set("load", probe) + + RecordPushedMetrics(client) + } + + series, err := ProbeMetricsOf("behind-nat", "load", now-3600, now, 300) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if len(series) != 1 { + t.Fatalf("Got %+v, expected one series", series) + } + if len(series[0].Points) != 5 { + t.Errorf("Got %d points, expected the five that were pushed", len(series[0].Points)) + } +} + +// One host's measurements must not answer for another's : the graph would show +// a machine that was never asked about. +func TestOneHostsMeasurementsDoNotAnswerForAnother(t *testing.T) { + setupHistoryTest(t) + + now := time.Now().Unix() + recordAt(t, "load", now-60, metric(1, nil)) + + client := newTestRemoteWigo("uuid-push", "behind-nat", "frontend") + probe := newTestProbe(client.LocalHost, "load", 100) + probe.Timestamp = now - 60 + probe.Metrics = []interface{}{metric(42, nil)} + client.LocalHost.Probes.Set("load", probe) + RecordPushedMetrics(client) + + local, err := ProbeMetricsOf(LocalWigo.GetHostname(), "load", now-3600, now, 300) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if len(local) != 1 || len(local[0].Points) != 1 || local[0].Points[0].Value != 1 { + t.Errorf("Got %+v, expected only what was measured here", local) + } + + pushed, err := ProbeMetricsOf("behind-nat", "load", now-3600, now, 300) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if len(pushed) != 1 || len(pushed[0].Points) != 1 || pushed[0].Points[0].Value != 42 { + t.Errorf("Got %+v, expected only what the client sent", pushed) + } + + // And a host nobody recorded anything for has nothing, rather than + // everything. + if other, err := ProbeMetricsOf("someone-else", "load", now-3600, now, 300); err != nil || len(other) != 0 { + t.Errorf("Got %+v (%v), expected nothing", other, err) + } +} + +// A database written before metrics carried a host has to gain the column, and +// what is already in it was measured here : nothing else could have been. +func TestAnOlderDatabaseGainsTheHostColumn(t *testing.T) { + setupHistoryTest(t) + + now := time.Now().Unix() + + // Put it back the way it was, rows included + if _, err := LocalWigo.sqlLiteConn.Exec(`DROP TABLE metrics;`); err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if _, err := LocalWigo.sqlLiteConn.Exec(` + CREATE TABLE metrics ( + id integer not null primary key, + probe text not null, + tags text not null, + value real not null, + at int not null + ) ;`); err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if _, err := LocalWigo.sqlLiteConn.Exec( + `INSERT INTO metrics(probe,tags,value,at) VALUES('load','',7,?);`, now-60); err != nil { + t.Fatalf("Unexpected error : %s", err) + } + + if err := migrateMetricsHost(LocalWigo.sqlLiteConn, LocalWigo.GetHostname()); err != nil { + t.Fatalf("The migration failed : %s", err) + } + + series, err := ProbeMetricsOf(LocalWigo.GetHostname(), "load", now-3600, now, 300) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if len(series) != 1 || len(series[0].Points) != 1 || series[0].Points[0].Value != 7 { + t.Errorf("Got %+v, expected the old row attributed to this host", series) + } + + // Twice is not an error : it runs at every startup. + if err := migrateMetricsHost(LocalWigo.sqlLiteConn, LocalWigo.GetHostname()); err != nil { + t.Errorf("Running it again failed : %s", err) + } +} + +// A client pushes far more often than its probes run, and each push carries the +// same result again. Writing it every time stored one measurement a dozen times +// -- the master's database growing for nothing, which is the cost the whole +// choice was weighed against. +func TestTheSameMeasurementPushedTwiceIsWrittenOnce(t *testing.T) { + setupHistoryTest(t) + + now := time.Now().Unix() + client := newTestRemoteWigo("uuid-push", "behind-nat", "frontend") + + probe := newTestProbe(client.LocalHost, "load", 100) + probe.Timestamp = now - 60 + probe.Metrics = []interface{}{metric(3, nil)} + client.LocalHost.Probes.Set("load", probe) + + // Twelve pushes between two runs of the probe, which is what a five second + // interval against a minute one gives. + for i := 0; i < 12; i++ { + RecordPushedMetrics(client) + } + + if rows := countMetricRows(t, "behind-nat", "load"); rows != 1 { + t.Errorf("Got %d rows, expected the measurement to be written once", rows) + } + + // And the next run is written, since it is a different measurement + next := newTestProbe(client.LocalHost, "load", 100) + next.Timestamp = now + next.Metrics = []interface{}{metric(4, nil)} + client.LocalHost.Probes.Set("load", next) + RecordPushedMetrics(client) + + if rows := countMetricRows(t, "behind-nat", "load"); rows != 2 { + t.Errorf("Got %d rows, expected the newer measurement to be kept too", rows) + } + + series, err := ProbeMetricsOf("behind-nat", "load", now-3600, now+60, 300) + if err != nil { + t.Fatalf("Unexpected error : %s", err) + } + if len(series) != 1 || len(series[0].Points) != 2 { + t.Errorf("Got %+v, expected the two measurements", series) + } +} + +// A result older than the last one seen is not a new measurement : it is the +// same push arriving out of order, or a client whose clock went back. +func TestAnOlderMeasurementDoesNotReopenTheDoor(t *testing.T) { + setupHistoryTest(t) + + now := time.Now().Unix() + client := newTestRemoteWigo("uuid-push", "behind-nat", "frontend") + + for _, at := range []int64{now, now - 120} { + probe := newTestProbe(client.LocalHost, "load", 100) + probe.Timestamp = at + probe.Metrics = []interface{}{metric(1, nil)} + client.LocalHost.Probes.Set("load", probe) + RecordPushedMetrics(client) + } + + if rows := countMetricRows(t, "behind-nat", "load"); rows != 1 { + t.Errorf("Got %d rows, expected only the newest to have been kept", rows) + } +} + +func countMetricRows(t *testing.T, hostname string, probe string) int { + t.Helper() + + var count int + row := LocalWigo.sqlLiteConn.QueryRow( + `SELECT count(*) FROM metrics WHERE host = ? AND probe = ?;`, hostname, probe) + if err := row.Scan(&count); err != nil { + t.Fatalf("Unexpected error : %s", err) + } + + return count +} diff --git a/src/wigo/openapi.yaml b/src/wigo/openapi.yaml index 5c155bd..3a51d44 100644 --- a/src/wigo/openapi.yaml +++ b/src/wigo/openapi.yaml @@ -324,9 +324,10 @@ paths: tags: [history] summary: The same, for any host of the tree description: | - Each wigo keeps its own history and only its own, so a master answers - this by asking the host. A host that pushes rather than being polled - cannot be asked, and says so. + A wigo keeps its own history, so a master answers this by asking a + polled host. A host that pushes cannot be asked -- it sits behind a + NAT -- so its measurements are kept here as they arrive, and answered + from this database instead. parameters: - { $ref: "#/components/parameters/Hostname" } - { $ref: "#/components/parameters/Probe" } diff --git a/src/wigo/push_server.go b/src/wigo/push_server.go index af4af52..d17b209 100644 --- a/src/wigo/push_server.go +++ b/src/wigo/push_server.go @@ -219,6 +219,11 @@ func (this *PushServer) Update(req UpdateRequest, reply *bool) (err error) { if req.ProbesSchedule != nil { SetClientProbesSchedule(req.Uuid, req.ProbesSchedule, req.SkippedProbes, req.DisableRecords) } + // Its measurements, kept here because it is the only place + // they can be read from : we cannot call a host behind a NAT, + // so asking it later is not an option. See history.go. + RecordPushedMetrics(wigo) + // TODO this should return an error LocalWigo.AddOrUpdateRemoteWigo(wigo) } From 436013cce33c9d424d43a2c4c51e4e0c5c5452c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Germain=20Carr=C3=A9?= Date: Sun, 30 Aug 2026 11:27:01 +0200 Subject: [PATCH 3/6] Stop the timeline tooltip shrinking towards the right edge An absolutely positioned box with only a left offset is squeezed into whatever room is left beside it : the same tooltip measured 556px wide at the left of the band and 192 at the right, wrapping into twice the height. Its size comes from its content now, and its position is clamped rather than flipped past a threshold -- on a phone it is as wide as the band, and no threshold fits that. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/js/components/StatusTimeline.vue | 53 ++++++++++++++++--- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/src/public/src/js/components/StatusTimeline.vue b/src/public/src/js/components/StatusTimeline.vue index 5a4825d..f11436b 100644 --- a/src/public/src/js/components/StatusTimeline.vue +++ b/src/public/src/js/components/StatusTimeline.vue @@ -112,6 +112,7 @@
@@ -136,7 +137,7 @@