From 7bd812d568495fd48346a50d77cbaf263bcd5b09 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 7 Sep 2026 05:06:13 -0700 Subject: [PATCH] feat: warn once rather than silently when the analytics host is unreachable or rejects the key, so a wrong POSTHOG_KEY does not look like a quiet week, and cover the client with tests that assert the cookieless ingestion contract against a stub capture host: the sentinel distinct id, the mode flag, the three hash inputs, and that no event property names a person --- internal/observability/analytics/analytics.go | 25 +++- .../observability/analytics/analytics_test.go | 126 ++++++++++++++++++ 2 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 internal/observability/analytics/analytics_test.go diff --git a/internal/observability/analytics/analytics.go b/internal/observability/analytics/analytics.go index 3c33e488..f7be5dd6 100644 --- a/internal/observability/analytics/analytics.go +++ b/internal/observability/analytics/analytics.go @@ -20,8 +20,10 @@ import ( "context" "encoding/json" "io" + "log" "net/http" "strings" + "sync" "time" "github.com/warmbly/warmbly/internal/observability/errs" @@ -47,6 +49,8 @@ type Client struct { key string host string http *http.Client + // warned bounds the failure log to one line per process; see warnOnce. + warned sync.Once } // New returns a client, or nil when no key is configured. Returning nil rather @@ -137,19 +141,34 @@ func (c *Client) post(body []byte) { req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.host+capturePath, bytes.NewReader(body)) if err != nil { - errs.CaptureException(err) + c.warnOnce("cannot build a capture request for %s: %v", c.host, err) return } req.Header.Set("Content-Type", "application/json") resp, err := c.http.Do(req) if err != nil { - // A capture that did not land is worth knowing about but is not worth - // an error event per request; the log is enough. + c.warnOnce("cannot reach the analytics host %s: %v", c.host, err) return } defer resp.Body.Close() // Drained so the connection can be reused; the response body is of no // interest beyond that. _, _ = io.Copy(io.Discard, resp.Body) + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + c.warnOnce("the analytics host %s rejected a capture with %s (check POSTHOG_KEY)", c.host, resp.Status) + } +} + +// warnOnce logs the first failure and nothing after it. +// +// A wrong key or an unreachable host fails on every single event, so logging +// each one would bury the instance's real logs under analytics noise. Logging +// none of them is worse: a misconfigured key would look exactly like a quiet +// week. One line, the first time, is the useful amount. +func (c *Client) warnOnce(format string, args ...any) { + c.warned.Do(func() { + log.Printf("product analytics disabled for this run: "+format, args...) + }) } diff --git a/internal/observability/analytics/analytics_test.go b/internal/observability/analytics/analytics_test.go new file mode 100644 index 00000000..a9d9a9b5 --- /dev/null +++ b/internal/observability/analytics/analytics_test.go @@ -0,0 +1,126 @@ +package analytics + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// captured is one request the stub PostHog received. +type captured struct { + APIKey string `json:"api_key"` + Event string `json:"event"` + DistinctID string `json:"distinct_id"` + Properties map[string]any `json:"properties"` +} + +// stub stands in for PostHog's capture API and hands back what it was sent. +func stub(t *testing.T) (*httptest.Server, chan captured) { + t.Helper() + got := make(chan captured, 4) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != capturePath { + t.Errorf("posted to %s, want %s", r.URL.Path, capturePath) + } + var c captured + if err := json.NewDecoder(r.Body).Decode(&c); err != nil { + t.Errorf("decode body: %v", err) + } + got <- c + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + return srv, got +} + +func waitFor(t *testing.T, got chan captured) captured { + t.Helper() + select { + case c := <-got: + return c + case <-time.After(3 * time.Second): + t.Fatal("no event reached the capture host") + return captured{} + } +} + +// The contract with PostHog's ingestion: the cookieless sentinel as the +// distinct id, the mode flag, and the three hash inputs. Get any of these +// wrong and the event lands as a separate visitor, or not at all. +func TestCaptureSendsTheCookielessContract(t *testing.T) { + srv, got := stub(t) + c := New("phc_test", srv.URL) + if c == nil { + t.Fatal("a configured key should produce a client") + } + + c.Capture("signup_completed", Request{ + IP: "203.0.113.9", + UserAgent: "Mozilla/5.0", + Host: "app.warmbly.com", + }, map[string]any{"utm_source": "newsletter"}) + + ev := waitFor(t, got) + if ev.APIKey != "phc_test" { + t.Errorf("api_key = %q", ev.APIKey) + } + if ev.Event != "signup_completed" { + t.Errorf("event = %q", ev.Event) + } + if ev.DistinctID != cookielessDistinctID { + t.Errorf("distinct_id = %q, want the cookieless sentinel %q", ev.DistinctID, cookielessDistinctID) + } + for key, want := range map[string]any{ + "$cookieless_mode": true, + "$ip": "203.0.113.9", + "$raw_user_agent": "Mozilla/5.0", + "$host": "app.warmbly.com", + "utm_source": "newsletter", + } { + if ev.Properties[key] != want { + t.Errorf("properties[%q] = %v, want %v", key, ev.Properties[key], want) + } + } +} + +// Nothing in an event may name a person: that is the whole basis for having no +// consent banner. This guards the property set against a future caller quietly +// adding an identifier. +func TestCaptureCarriesNoIdentifiers(t *testing.T) { + srv, got := stub(t) + c := New("phc_test", srv.URL) + c.Capture("trial_started", Request{IP: "203.0.113.9", Host: "app.warmbly.com"}, nil) + + ev := waitFor(t, got) + for _, forbidden := range []string{"user_id", "organization_id", "org_id", "email", "distinct_id", "$user_id"} { + if _, ok := ev.Properties[forbidden]; ok { + t.Errorf("event carries %q, which would defeat cookieless mode", forbidden) + } + } +} + +// No key is the self-host default, and it has to mean no client and no request +// rather than a client that quietly points at PostHog Cloud. +func TestNewWithoutAKeyIsOffAndSafeToCall(t *testing.T) { + for _, key := range []string{"", " "} { + if c := New(key, ""); c != nil { + t.Fatalf("New(%q) returned a client; analytics must be off without a key", key) + } + } + // A nil client is the "never wired" shape and must not panic. + var c *Client + c.Capture("signup_completed", Request{}, nil) +} + +// An unset host must resolve to EU cloud, and a trailing slash must not produce +// a double slash in the capture path. +func TestHostDefaultingAndTrimming(t *testing.T) { + if got := New("k", "").host; got != DefaultHost { + t.Errorf("default host = %q, want %q", got, DefaultHost) + } + if got := New("k", "https://ph.example.com/").host; got != "https://ph.example.com" { + t.Errorf("trailing slash not trimmed: %q", got) + } +}