Fix: Do not collect telemetry if set to off

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

CHANGELOG.md

@@ -21,6 +21,7 @@ We use the following categories for changes:
   requests [#1205]. 
 
 ### Fixed
+- Do not collect telemetry if `timescaledb.telemetry_level=off` [#1612]
 - Fix broken cache eviction in clockcache [#1603]
 - Possible goroutine leak due to unbuffered channel in select block [#1604]
 

pkg/runner/runner.go

@@ -177,9 +177,13 @@ func Run(cfg *Config) error {
 		return fmt.Errorf("generate router: %w", err)
 	}
 
-	telemetryEngine := initTelemetryEngine(client)
-	telemetryEngine.Start()
-	defer telemetryEngine.Stop()
+	telemetryEngine, err := initTelemetryEngine(client)
+	if err != nil {
+		log.Debug("msg", "error starting telemetry engine", "error", err.Error())
+	} else {
+		telemetryEngine.Start()
+		defer telemetryEngine.Stop()
+	}
 
 	if len(cfg.ThanosStoreAPIListenAddr) > 0 {
 		srv := thanos.NewStorage(client.Queryable())
@@ -316,11 +320,11 @@ func Run(cfg *Config) error {
 	return err
 }
 
-func initTelemetryEngine(client *pgclient.Client) telemetry.Engine {
+func initTelemetryEngine(client *pgclient.Client) (telemetry.Engine, error) {
 	t, err := telemetry.NewEngine(client.ReadOnlyConnection(), PromscaleID, client.Queryable())
 	if err != nil {
 		log.Debug("msg", "err creating telemetry engine", "err", err.Error())
-		return t
+		return nil, err
 	}
 	if err := api.RegisterTelemetryMetrics(t); err != nil {
 		log.Error("msg", "error registering metrics for API telemetry", "err", err.Error())
@@ -334,5 +338,5 @@ func initTelemetryEngine(client *pgclient.Client) telemetry.Engine {
 	if err := rules.RegisterForTelemetry(t); err != nil {
 		log.Error("msg", "error registering metrics for rules telemetry", "err", err.Error())
 	}
-	return t
+	return t, nil
 }

pkg/telemetry/telemetry.go

@@ -44,6 +44,8 @@ type engineImpl struct {
 	uuid [16]byte
 	conn pgxconn.PgxConn
 
+	wasStartupMetadataWritten bool
+
 	stop chan struct{}
 
 	promqlEngine    *promql.Engine
@@ -54,18 +56,6 @@ type engineImpl struct {
 }
 
 func NewEngine(conn pgxconn.PgxConn, uuid [16]byte, promqlQueryable promql.Queryable) (Engine, error) {
-	isTelemetryOff, err := isTelemetryOff(conn)
-	if err != nil {
-		log.Debug("msg", "unable to get TimescaleDB telemetry configuration. Maybe TimescaleDB is not installed", "err", err.Error())
-		return NewNoopEngine(), nil
-	}
-	if isTelemetryOff {
-		return NewNoopEngine(), nil
-	}
-	// Warn the users about telemetry collection only if telemetry collection is enabled.
-	log.Warn("msg", "Promscale collects anonymous usage telemetry data to help the Promscale team better understand and assist users. "+
-		"This can be disabled via the process described at https://docs.timescale.com/timescaledb/latest/how-to-guides/configuration/telemetry/#disabling-telemetry")
-
 	t := &engineImpl{
 		conn: conn,
 		uuid: uuid,
@@ -81,26 +71,29 @@ func NewEngine(conn pgxconn.PgxConn, uuid [16]byte, promqlQueryable promql.Query
 		promqlQueryable: promqlQueryable,
 	}
 
-	if err := t.writeStartupMetadata(); err != nil {
-		return NewNoopEngine(), fmt.Errorf("writing metadata: %w", err)
+	if t.IsActive() {
+		// Warn the users about telemetry collection only if telemetry collection is enabled.
+		log.Warn("msg", "Promscale collects anonymous usage telemetry data to help the Promscale team better understand and assist users. "+
+			"This can be disabled via the process described at https://docs.timescale.com/timescaledb/latest/how-to-guides/configuration/telemetry/#disabling-telemetry")
+
+		if err := t.writeStartupMetadata(); err != nil {
+			return nil, fmt.Errorf("writing metadata: %w", err)
+		}
 	}
 	return t, nil
 }
 
-func isTelemetryOff(conn pgxconn.PgxConn) (bool, error) {
-	if !util.IsTimescaleDBInstalled(conn) {
-		return true, nil
+func (t *engineImpl) IsActive() bool {
+	if !util.IsTimescaleDBInstalled(t.conn) {
+		return false
 	}
 	var state string
-	err := conn.QueryRow(context.Background(), "SHOW timescaledb.telemetry_level").Scan(&state)
-	if err != nil {
-		// Return true as telemetry is by default not collected when TimescaleDB is not installed.
-		return true, fmt.Errorf("fetching timescaledb telemetry setting: %w", err)
-	}
-	if state == "off" {
-		return true, nil
+	err := t.conn.QueryRow(context.Background(), "SHOW timescaledb.telemetry_level").Scan(&state)
+	if err != nil || state == "off" {
+		// Return false as telemetry is by default not collected when TimescaleDB is not installed.
+		return false
 	}
-	return false, nil
+	return true
 }
 
 // writeMetadata writes Promscale and Tobs metadata.
@@ -112,6 +105,7 @@ func (t *engineImpl) writeStartupMetadata() error {
 	if len(tobs) > 0 {
 		t.writeToTimescaleMetadataTable(tobs)
 	}
+	t.wasStartupMetadataWritten = true
 	return nil
 }
 
@@ -202,6 +196,16 @@ func (t *engineImpl) syncWithMetadataTable(queryFormat string, m Metadata) error
 }
 
 func (t *engineImpl) Sync() error {
+	if !t.IsActive() {
+		log.Debug("msg", "skipping telemetry", "reason", "telemetry is inactive")
+		return nil
+	}
+	if !t.wasStartupMetadataWritten {
+		if err := t.writeStartupMetadata(); err != nil {
+			log.Debug("msg", "writing telemetry metadata", "err", err.Error())
+		}
+	}
+	log.Debug("msg", "updating telemetry")
 	if err := t.syncWithInfoTable(); err != nil {
 		return fmt.Errorf("sync info table: %w", err)
 	}
@@ -449,11 +453,3 @@ func (t *engineImpl) syncPromqlTelemetry() {
 func convertIntToString(i int64) string {
 	return strconv.FormatInt(i, 10)
 }
-
-type Noop struct{}
-
-func NewNoopEngine() Engine                                          { return Noop{} }
-func (Noop) Start()                                                  {}
-func (Noop) Stop()                                                   {}
-func (Noop) RegisterDynamicMetadata(string, prometheus.Metric) error { return nil }
-func (Noop) RegisterMetric(string, ...prometheus.Metric) error       { return nil }

pkg/tests/end_to_end_tests/telemetry_test.go

@@ -38,6 +38,7 @@ func setTobsEnv(prop string) error {
 type e2eTelemetry interface {
 	telemetry.Engine
 	Sync() error
+	IsActive() bool
 }
 
 func telemetryEngineForE2E(t testing.TB, conn pgxconn.PgxConn, qryable promql.Queryable) e2eTelemetry {
@@ -263,13 +264,25 @@ func TestTelemetryEngineWhenTelemetryIsSetToOff(t *testing.T) {
 		_, err2 := conn.Exec(context.Background(), "SELECT set_config('timescaledb.telemetry_level', 'off', false)")
 		require.NoError(t, err2)
 
-		engine, err := telemetry.NewEngine(conn, generateUUID(), nil)
-		require.NoError(t, err)
-		_, isNoop := engine.(telemetry.Noop)
-		require.True(t, isNoop)
+		testIfTelemetryIsOff(t, conn)
 	})
 }
 
+func testIfTelemetryIsOff(t testing.TB, conn pgxconn.PgxConn) {
+	engine := telemetryEngineForE2E(t, conn, nil)
+
+	numTelemetryWritten := 0
+	err := conn.QueryRow(context.Background(), "SELECT count(*) FROM _timescaledb_catalog.metadata;").Scan(&numTelemetryWritten)
+	require.NoError(t, err)
+
+	require.NoError(t, engine.Sync())
+	numTelemetryAfterSync := 0
+	err = conn.QueryRow(context.Background(), "SELECT count(*) FROM _timescaledb_catalog.metadata;").Scan(&numTelemetryAfterSync)
+	require.NoError(t, err)
+
+	require.Equal(t, numTelemetryWritten, numTelemetryAfterSync)
+}
+
 func TestTelemetrySQLStats(t *testing.T) {
 	if !*useTimescaleDB {
 		t.Skip("telemetry requires TimescaleDB")
@@ -320,14 +333,8 @@ func TestTelemetryWithoutTimescaleDB(t *testing.T) {
 		db := testhelpers.PgxPoolWithRole(t, *testDatabase, "prom_admin")
 		defer db.Close()
 
-		conn := pgxconn.NewPgxConn(db)
-
-		engine, err := telemetry.NewEngine(conn, generateUUID(), nil)
-		require.NoError(t, err)
-
-		// If TimescaleDB is unavailable, telemetry should be a noop.
-		_, isNoop := engine.(telemetry.Noop)
-		require.True(t, isNoop)
+		engine := telemetryEngineForE2E(t, pgxconn.NewPgxConn(db), nil)
+		require.False(t, engine.IsActive())
 	})
 }