Repository: cloudprober/cloudprober

Issue:
[surfacers.prometheus] Consider either deleting stale gauge metrics by default or always include timestamp for them
We recently made some changes to prometheus surfacer which encourage timestamp-less exporting, the reason being:

a) if you include timestamp, you'll have to make sure you don't export metrics with timestamps older than 10m, that causes warnings in prometheus metrics scraper. Also this behavior is not right for cumulative metrics if they update infrequently -- prometheus will show gap in data, and sometimes even expire the timeseries if gap is too large.

b) if you don't include timestamps, stale metrics warning goes away, but prometheus continues to consider the scraped value as the current value.

Note that the latter behavior (no timestamp) is fine for cumulative metrics, as it's easy to identify that the value has not changed. But for gauge metrics it's problematic -- they are true at a particular time, not 15 min later for example. 

We should make this behavior explicit in configuration. Maybe gauge metrics should always carry timestamps and their stale values should always be deleted, regardless of the config.

Discussion:
@manugarg: My proposal will be to change the default behavior of `include_timestamp` based on type of metrics -- for cumulative metrics timestamp will not be included and for gauge it will be included.
https://github.com/cloudprober/cloudprober/blob/5590ff7c720f430862040ffdd8c4dd679dde9e21/surfacers/internal/prometheus/proto/config.proto#L15


@manugarg: #1169 should do it.@manugarg: #1169 should fix it.

Solution patch:
diff --git a/surfacers/internal/prometheus/prometheus.go b/surfacers/internal/prometheus/prometheus.go
index e78f3f8b..3102ea56 100644
--- a/surfacers/internal/prometheus/prometheus.go
+++ b/surfacers/internal/prometheus/prometheus.go
@@ -1,4 +1,4 @@
-// Copyright 2017-2021 The Cloudprober Authors.
+// Copyright 2017-2025 The Cloudprober Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -52,7 +52,7 @@ import (
 
 var (
 	metricsPrefixFlag    = flag.String("prometheus_metrics_prefix", "", "Metrics prefix")
-	includeTimestampFlag = flag.Bool("prometheus_include_timestamp", configpb.Default_SurfacerConf_IncludeTimestamp, "Include timestamp in metrics")
+	includeTimestampFlag = flag.Bool("prometheus_include_timestamp", false, "Include timestamp in metrics")
 )
 
 // Prometheus metric and label names should match the following regular
@@ -104,6 +104,42 @@ type httpWriter struct {
 	doneChan chan struct{}
 }
 
+// defaultBoolEnum is used to record if a boolean flag (or config option) was
+// set explicitly or not.
+type defaultBoolEnum int8
+
+const (
+	defaultBehavior defaultBoolEnum = 0 // not set explicitly
+	explicitTrue    defaultBoolEnum = 1
+	explicitFalse   defaultBoolEnum = 2
+)
+
+var defaultBoolMap = map[bool]defaultBoolEnum{
+	false: explicitFalse,
+	true:  explicitTrue,
+}
+
+func shouldIncludeTimestamp(c *configpb.SurfacerConf) defaultBoolEnum {
+	// Config option has highest priority if set explicitly.
+	if c.IncludeTimestamp != nil {
+		return defaultBoolMap[c.GetIncludeTimestamp()]
+	}
+
+	// Next check if flag was set explicitly.
+	found := false
+	flag.Visit(func(f *flag.Flag) {
+		if f.Name == "prometheus_include_timestamp" {
+			found = true
+			return
+		}
+	})
+	if found {
+		return defaultBoolMap[*includeTimestampFlag]
+	}
+
+	return defaultBehavior
+}
+
 // PromSurfacer implements a prometheus surfacer for Cloudprober. PromSurfacer
 // organizes metrics into a two-level data structure:
 //  1. Metric name -> PromMetric data structure dict.
@@ -113,15 +149,16 @@ type httpWriter struct {
 //
 // Data key represents a unique combination of metric name and labels.
 type PromSurfacer struct {
-	c                *configpb.SurfacerConf // Configuration
-	opts             *options.Options
-	includeTimestamp bool
-	prefix           string                     // Metrics prefix, e.g. "cloudprober_"
-	emChan           chan *metrics.EventMetrics // Buffered channel to store incoming EventMetrics
-	metrics          map[string]*promMetric     // Metric name to promMetric mapping
-	metricNames      []string                   // Metric names, to keep names ordered.
-	queryChan        chan *httpWriter           // Query channel
-	l                *logger.Logger
+	c                        *configpb.SurfacerConf // Configuration
+	opts                     *options.Options
+	includeTimestamp         defaultBoolEnum
+	disableMetricsExpiration defaultBoolEnum
+	prefix                   string                     // Metrics prefix, e.g. "cloudprober_"
+	emChan                   chan *metrics.EventMetrics // Buffered channel to store incoming EventMetrics
+	metrics                  map[string]*promMetric     // Metric name to promMetric mapping
+	metricNames              []string                   // Metric names, to keep names ordered.
+	queryChan                chan *httpWriter           // Query channel
+	l                        *logger.Logger
 
 	// A handler that takes a promMetric and a dataKey and writes the
 	// corresponding metric string to the provided io.Writer.
@@ -140,36 +177,44 @@ func New(ctx context.Context, config *configpb.SurfacerConf, opts *options.Optio
 		config = &configpb.SurfacerConf{}
 	}
 	ps := &PromSurfacer{
-		c:                config,
-		opts:             opts,
-		emChan:           make(chan *metrics.EventMetrics, config.GetMetricsBufferSize()),
-		queryChan:        make(chan *httpWriter, queriesQueueSize),
-		metrics:          make(map[string]*promMetric),
-		metricNameRe:     regexp.MustCompile(ValidMetricNameRegex),
-		labelNameRe:      regexp.MustCompile(ValidLabelNameRegex),
-		includeTimestamp: *includeTimestampFlag,
-		prefix:           *metricsPrefixFlag,
-		l:                l,
+		c:            config,
+		opts:         opts,
+		emChan:       make(chan *metrics.EventMetrics, config.GetMetricsBufferSize()),
+		queryChan:    make(chan *httpWriter, queriesQueueSize),
+		metrics:      make(map[string]*promMetric),
+		metricNameRe: regexp.MustCompile(ValidMetricNameRegex),
+		labelNameRe:  regexp.MustCompile(ValidLabelNameRegex),
+		prefix:       *metricsPrefixFlag,
+		l:            l,
 	}
 
-	if ps.c.IncludeTimestamp != nil {
-		ps.includeTimestamp = ps.c.GetIncludeTimestamp()
-	}
+	ps.includeTimestamp = shouldIncludeTimestamp(ps.c)
 
 	if ps.c.MetricsPrefix != nil {
 		ps.prefix = ps.c.GetMetricsPrefix()
 	}
 
-	if ps.includeTimestamp {
+	switch ps.includeTimestamp {
+	case explicitTrue:
 		ps.dataWriter = func(w io.Writer, pm *promMetric, k string) {
 			fmt.Fprintf(w, "%s %s %d\n", k, pm.data[k].value, pm.data[k].timestamp)
 		}
-	} else {
+	case explicitFalse:
 		ps.dataWriter = func(w io.Writer, pm *promMetric, k string) {
 			fmt.Fprintf(w, "%s %s\n", k, pm.data[k].value)
 		}
+	default:
+		ps.dataWriter = func(w io.Writer, pm *promMetric, k string) {
+			if pm.typ == "gauge" {
+				fmt.Fprintf(w, "%s %s %d\n", k, pm.data[k].value, pm.data[k].timestamp)
+				return
+			}
+			fmt.Fprintf(w, "%s %s\n", k, pm.data[k].value)
+		}
 	}
 
+	ps.disableMetricsExpiration = ps.shouldDisableMetricsExpiration()
+
 	// Start a goroutine to process the incoming EventMetrics as well as
 	// the incoming web queries. To avoid data access race conditions, we do
 	// one thing at a time.
@@ -188,9 +233,7 @@ func New(ctx context.Context, config *configpb.SurfacerConf, opts *options.Optio
 				ps.writeData(hw.w)
 				close(hw.doneChan)
 			case <-staleMetricDeleteTimer.C:
-				if !ps.disableMetricsExpiration() {
-					ps.deleteExpiredMetrics()
-				}
+				ps.deleteExpiredMetrics()
 			}
 		}
 	}()
@@ -221,16 +264,18 @@ func (ps *PromSurfacer) Write(_ context.Context, em *metrics.EventMetrics) {
 	}
 }
 
-func (ps *PromSurfacer) disableMetricsExpiration() bool {
+func (ps *PromSurfacer) shouldDisableMetricsExpiration() defaultBoolEnum {
 	if ps.c != nil && ps.c.DisableMetricsExpiration != nil {
-		return ps.c.GetDisableMetricsExpiration()
+		return defaultBoolMap[*ps.c.DisableMetricsExpiration]
 	}
 
-	if !ps.includeTimestamp {
-		return true
-	}
-
-	return false
+	// If we are never including timestamp, we disable metrics expiration
+	// and vice versa.
+	return map[defaultBoolEnum]defaultBoolEnum{
+		explicitFalse:   explicitTrue,
+		explicitTrue:    explicitFalse,
+		defaultBehavior: defaultBehavior,
+	}[ps.includeTimestamp]
 }
 
 func promType(em *metrics.EventMetrics) string {
@@ -436,11 +481,20 @@ func (ps *PromSurfacer) writeData(w io.Writer) {
 // Note from manugarg: We can possibly optimize this by recording expired
 // keys while serving the metrics, and deleting them based on the timer.
 func (ps *PromSurfacer) deleteExpiredMetrics() {
+	if ps.disableMetricsExpiration == explicitTrue {
+		return
+	}
+
 	staleTimeThreshold := promTime(time.Now()) - metricExpirationTime.Milliseconds()
 
 	for _, name := range ps.metricNames {
 		pm := ps.metrics[name]
 
+		// For default behavior, we don't expire counter metrics.
+		if ps.disableMetricsExpiration == defaultBehavior && pm.typ == "counter" {
+			continue
+		}
+
 		var expiredMetricsKeys []string
 		for metricKey, v := range pm.data {
 			if v.timestamp < staleTimeThreshold {
diff --git a/surfacers/internal/prometheus/proto/config.pb.go b/surfacers/internal/prometheus/proto/config.pb.go
index d94a5dd3..9f98eabf 100644
--- a/surfacers/internal/prometheus/proto/config.pb.go
+++ b/surfacers/internal/prometheus/proto/config.pb.go
@@ -29,22 +29,30 @@ type SurfacerConf struct {
 	// NOTE: This field is confusing for users and will be removed from the config
 	// after v0.10.3.
 	MetricsBufferSize *int64 `protobuf:"varint,1,opt,name=metrics_buffer_size,json=metricsBufferSize,def=10000" json:"metrics_buffer_size,omitempty"`
-	// Whether to include timestamps in metrics. If enabled (default) each metric
-	// string includes the metric timestamp as recorded in the EventMetric.
-	// Prometheus associates the scraped values with this timestamp. If disabled,
-	// i.e. timestamps are not exported, prometheus associates scraped values with
-	// scrape timestamp.
+	// Whether to include timestamps in metrics. By default, we include
+	// timestamps only for gauge metrics as they represent the value at a certain
+	// point in time.
+	//
+	// If explicitly enabled, timestamp will be attached to all metrics,
+	// including counters. If explicitly disabled, timestamp will not be attached
+	// to gauges as well.
 	//
 	// As it's typically useful to set this across the deployment, this field can
 	// also be set through the command line flag --prometheus_include_timestamp.
 	// If both are set, the config value takes precedence.
-	IncludeTimestamp *bool `protobuf:"varint,2,opt,name=include_timestamp,json=includeTimestamp,def=1" json:"include_timestamp,omitempty"`
-	// We automatically delete metrics that haven't been updated for more than
-	// 10 min. We do that because prometheus generates warnings while scraping
-	// metrics that are more than 10m old and prometheus knows that metrics are
-	// more than 10m old because we include timestamp in metrics (by default).
-	// This option controls that behavior.
-	// It's automatically set to true if include_timestamp is false.
+	IncludeTimestamp *bool `protobuf:"varint,2,opt,name=include_timestamp,json=includeTimestamp" json:"include_timestamp,omitempty"`
+	// Whether to disable all metrics expiration.
+	//
+	// Default behavior (recommended):
+	// By default, our goal is to expire timestamped metrics, which are more
+	// than 10 minutes old (see why below). If include_timestamp is not explicitly
+	// set to true or false, only gauge metrics will have timestamps, and only
+	// they will be expired. Similarly, if include_timestamp is explicitly set to
+	// false, no metrics will be expired, and if include_timestamp is explicitly
+	// set to true, all metrics will be expired.
+	//
+	// Why do we expire timestamped metrics? Prometheus generates warnings while
+	// scraping metrics that have timestamps that are more than 10m old.
 	DisableMetricsExpiration *bool `protobuf:"varint,5,opt,name=disable_metrics_expiration,json=disableMetricsExpiration" json:"disable_metrics_expiration,omitempty"`
 	// URL that prometheus scrapes metrics from.
 	MetricsUrl *string `protobuf:"bytes,3,opt,name=metrics_url,json=metricsUrl,def=/metrics" json:"metrics_url,omitempty"`
@@ -63,7 +71,6 @@ type SurfacerConf struct {
 // Default values for SurfacerConf fields.
 const (
 	Default_SurfacerConf_MetricsBufferSize = int64(10000)
-	Default_SurfacerConf_IncludeTimestamp  = bool(true)
 	Default_SurfacerConf_MetricsUrl        = string("/metrics")
 )
 
@@ -108,7 +115,7 @@ func (x *SurfacerConf) GetIncludeTimestamp() bool {
 	if x != nil && x.IncludeTimestamp != nil {
 		return *x.IncludeTimestamp
 	}
-	return Default_SurfacerConf_IncludeTimestamp
+	return false
 }
 
 func (x *SurfacerConf) GetDisableMetricsExpiration() bool {
@@ -136,10 +143,10 @@ var File_github_com_cloudprober_cloudprober_surfacers_internal_prometheus_proto_
 
 const file_github_com_cloudprober_cloudprober_surfacers_internal_prometheus_proto_config_proto_rawDesc = "" +
 	"\n" +
-	"Sgithub.com/cloudprober/cloudprober/surfacers/internal/prometheus/proto/config.proto\x12\x1fcloudprober.surfacer.prometheus\"\x88\x02\n" +
+	"Sgithub.com/cloudprober/cloudprober/surfacers/internal/prometheus/proto/config.proto\x12\x1fcloudprober.surfacer.prometheus\"\x82\x02\n" +
 	"\fSurfacerConf\x125\n" +
-	"\x13metrics_buffer_size\x18\x01 \x01(\x03:\x0510000R\x11metricsBufferSize\x121\n" +
-	"\x11include_timestamp\x18\x02 \x01(\b:\x04trueR\x10includeTimestamp\x12<\n" +
+	"\x13metrics_buffer_size\x18\x01 \x01(\x03:\x0510000R\x11metricsBufferSize\x12+\n" +
+	"\x11include_timestamp\x18\x02 \x01(\bR\x10includeTimestamp\x12<\n" +
 	"\x1adisable_metrics_expiration\x18\x05 \x01(\bR\x18disableMetricsExpiration\x12)\n" +
 	"\vmetrics_url\x18\x03 \x01(\t:\b/metricsR\n" +
 	"metricsUrl\x12%\n" +
diff --git a/surfacers/internal/prometheus/proto/config.proto b/surfacers/internal/prometheus/proto/config.proto
index cec8efa5..e5b06c99 100644
--- a/surfacers/internal/prometheus/proto/config.proto
+++ b/surfacers/internal/prometheus/proto/config.proto
@@ -12,23 +12,31 @@ message SurfacerConf {
   // after v0.10.3.
   optional int64 metrics_buffer_size = 1 [default = 10000];
 
-  // Whether to include timestamps in metrics. If enabled (default) each metric
-  // string includes the metric timestamp as recorded in the EventMetric.
-  // Prometheus associates the scraped values with this timestamp. If disabled,
-  // i.e. timestamps are not exported, prometheus associates scraped values with
-  // scrape timestamp.
+  // Whether to include timestamps in metrics. By default, we include
+  // timestamps only for gauge metrics as they represent the value at a certain
+  // point in time.
+  //
+  // If explicitly enabled, timestamp will be attached to all metrics,
+  // including counters. If explicitly disabled, timestamp will not be attached
+  // to gauges as well.
   //
   // As it's typically useful to set this across the deployment, this field can
   // also be set through the command line flag --prometheus_include_timestamp.
   // If both are set, the config value takes precedence.
-  optional bool include_timestamp = 2 [default = true];
+  optional bool include_timestamp = 2;
 
-  // We automatically delete metrics that haven't been updated for more than
-  // 10 min. We do that because prometheus generates warnings while scraping
-  // metrics that are more than 10m old and prometheus knows that metrics are
-  // more than 10m old because we include timestamp in metrics (by default).
-  // This option controls that behavior.
-  // It's automatically set to true if include_timestamp is false.
+  // Whether to disable all metrics expiration.
+  //
+  // Default behavior (recommended):
+  // By default, our goal is to expire timestamped metrics, which are more
+  // than 10 minutes old (see why below). If include_timestamp is not explicitly
+  // set to true or false, only gauge metrics will have timestamps, and only
+  // they will be expired. Similarly, if include_timestamp is explicitly set to
+  // false, no metrics will be expired, and if include_timestamp is explicitly
+  // set to true, all metrics will be expired.
+  //
+  // Why do we expire timestamped metrics? Prometheus generates warnings while
+  // scraping metrics that have timestamps that are more than 10m old.
   optional bool disable_metrics_expiration = 5;
 
   // URL that prometheus scrapes metrics from.



Test patch:
diff --git a/surfacers/internal/prometheus/prometheus_test.go b/surfacers/internal/prometheus/prometheus_test.go
index 51a5b4b3..19b121ca 100644
--- a/surfacers/internal/prometheus/prometheus_test.go
+++ b/surfacers/internal/prometheus/prometheus_test.go
@@ -1,4 +1,4 @@
-// Copyright 2017-2020 The Cloudprober Authors.
+// Copyright 2017-2025 The Cloudprober Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -17,9 +17,11 @@ package prometheus
 import (
 	"bytes"
 	"context"
+	"flag"
 	"fmt"
 	"math/rand"
 	"net/http"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -182,7 +184,7 @@ func TestInvalidNames(t *testing.T) {
 	verify(t, ps, expectedMetrics)
 }
 
-func testWebOutput(t *testing.T, config *configpb.SurfacerConf, expectTimestamp bool) {
+func testWebOutput(t *testing.T, config *configpb.SurfacerConf, expectTimestamp string) {
 	t.Helper()
 
 	ps := testPromSurfacerNoErr(t, config)
@@ -190,32 +192,48 @@ func testWebOutput(t *testing.T, config *configpb.SurfacerConf, expectTimestamp
 	latencyVal.AddSample(0.5)
 	latencyVal.AddSample(5)
 	ts := time.Now()
-	ps.record(metrics.NewEventMetrics(ts).
+	counterEM := metrics.NewEventMetrics(ts).
 		AddMetric("sent", metrics.NewInt(32)).
-		AddMetric("rcvd", metrics.NewInt(22)).
 		AddMetric("latency", latencyVal).
 		AddMetric("resp_code", metrics.NewMap("code").IncKeyBy("200", 19)).
-		AddLabel("ptype", "http"))
+		AddLabel("ptype", "http")
+	ps.record(counterEM)
+
+	gaugeEM := metrics.NewEventMetrics(ts).
+		AddMetric("num_goroutines", metrics.NewInt(22)).
+		AddLabel("system", "sysvars")
+	gaugeEM.Kind = metrics.GAUGE
+	ps.record(gaugeEM)
+
 	var b bytes.Buffer
 	ps.writeData(&b)
 	data := b.String()
-	var tsSuffix string
-	if expectTimestamp {
-		tsSuffix = " " + fmt.Sprintf("%d", ts.UnixNano()/(1000*1000))
+	var counterSuffix string
+	var gaugeSuffix string
+	tsSuffix := fmt.Sprintf(" %d", ts.UnixNano()/(1000*1000))
+	switch expectTimestamp {
+	case "default":
+		gaugeSuffix = tsSuffix
+	case "true":
+		counterSuffix = tsSuffix
+		gaugeSuffix = tsSuffix
+	case "false":
+		counterSuffix = ""
+		gaugeSuffix = ""
 	}
 	for _, d := range []string{
 		"# TYPE sent counter",
-		"# TYPE rcvd counter",
 		"# TYPE resp_code counter",
 		"# TYPE latency histogram",
-		"sent{ptype=\"http\"} 32" + tsSuffix,
-		"rcvd{ptype=\"http\"} 22" + tsSuffix,
-		"resp_code{ptype=\"http\",code=\"200\"} 19" + tsSuffix,
-		"latency_sum{ptype=\"http\"} 5.5" + tsSuffix,
-		"latency_count{ptype=\"http\"} 2" + tsSuffix,
-		"latency_bucket{ptype=\"http\",le=\"1\"} 1" + tsSuffix,
-		"latency_bucket{ptype=\"http\",le=\"4\"} 1" + tsSuffix,
-		"latency_bucket{ptype=\"http\",le=\"+Inf\"} 2" + tsSuffix,
+		"sent{ptype=\"http\"} 32" + counterSuffix,
+		"resp_code{ptype=\"http\",code=\"200\"} 19" + counterSuffix,
+		"latency_sum{ptype=\"http\"} 5.5" + counterSuffix,
+		"latency_count{ptype=\"http\"} 2" + counterSuffix,
+		"latency_bucket{ptype=\"http\",le=\"1\"} 1" + counterSuffix,
+		"latency_bucket{ptype=\"http\",le=\"4\"} 1" + counterSuffix,
+		"latency_bucket{ptype=\"http\",le=\"+Inf\"} 2" + counterSuffix,
+		"# TYPE num_goroutines gauge",
+		"num_goroutines{system=\"sysvars\"} 22" + gaugeSuffix,
 	} {
 		if !strings.Contains(data, d+"\n") {
 			t.Errorf("String \"%s\" not found in output data: %s", d, data)
@@ -228,23 +246,17 @@ func TestScrapeOutput(t *testing.T) {
 	oldIncludeTimestampFlag := *includeTimestampFlag
 
 	t.Run("IncludeTimestamp config default", func(t *testing.T) {
-		testWebOutput(t, nil, true)
+		testWebOutput(t, nil, "default")
 	})
 
 	t.Run("IncludeTimestamp config true", func(t *testing.T) {
 		defer func() { *includeTimestampFlag = oldIncludeTimestampFlag }()
 		*includeTimestampFlag = false
-		testWebOutput(t, &configpb.SurfacerConf{IncludeTimestamp: proto.Bool(true)}, true)
+		testWebOutput(t, &configpb.SurfacerConf{IncludeTimestamp: proto.Bool(true)}, "true")
 	})
 
 	t.Run("IncludeTimestamp config false", func(t *testing.T) {
-		testWebOutput(t, &configpb.SurfacerConf{IncludeTimestamp: proto.Bool(false)}, false)
-	})
-
-	t.Run("IncludeTimestamp flag false", func(t *testing.T) {
-		defer func() { *includeTimestampFlag = oldIncludeTimestampFlag }()
-		*includeTimestampFlag = false
-		testWebOutput(t, nil, false)
+		testWebOutput(t, &configpb.SurfacerConf{IncludeTimestamp: proto.Bool(false)}, "false")
 	})
 }
 
@@ -372,62 +384,119 @@ func TestMain(m *testing.M) {
 
 func TestDisableMetricsExpiration(t *testing.T) {
 	tests := []struct {
-		name                     string
-		includeTimestamp         bool
+		includeTimestamp         []defaultBoolEnum
 		disableMetricsExpiration *bool
-		want                     bool
+		want                     []defaultBoolEnum
 	}{
+		// Not explicitly set
 		{
-			name:                     "DisableMetricsExpiration unset, IncludeTimestamp true",
-			includeTimestamp:         true,
-			disableMetricsExpiration: nil,
-			want:                     false,
-		},
-		{
-			name:                     "DisableMetricsExpiration unset, IncludeTimestamp false",
-			includeTimestamp:         false,
-			disableMetricsExpiration: nil,
-			want:                     true,
+			includeTimestamp: []defaultBoolEnum{defaultBehavior, explicitTrue, explicitFalse},
+			want:             []defaultBoolEnum{defaultBehavior, explicitFalse, explicitTrue},
 		},
 		{
-			name:                     "DisableMetricsExpiration true, IncludeTimestamp true",
-			includeTimestamp:         true,
+			includeTimestamp:         []defaultBoolEnum{defaultBehavior, explicitTrue, explicitFalse},
 			disableMetricsExpiration: proto.Bool(true),
-			want:                     true,
+			want:                     []defaultBoolEnum{explicitTrue, explicitTrue, explicitTrue},
 		},
 		{
-			name:                     "DisableMetricsExpiration false, IncludeTimestamp true",
-			includeTimestamp:         true,
+			includeTimestamp:         []defaultBoolEnum{defaultBehavior, explicitTrue, explicitFalse},
 			disableMetricsExpiration: proto.Bool(false),
-			want:                     false,
+			want:                     []defaultBoolEnum{explicitFalse, explicitFalse, explicitFalse},
 		},
+	}
+
+	for _, tt := range tests {
+		for i, includeTimestamp := range tt.includeTimestamp {
+			t.Run(fmt.Sprintf("includeTimestamp=%v, disableMetricsExpiration=%v", includeTimestamp, tt.disableMetricsExpiration), func(t *testing.T) {
+				conf := &configpb.SurfacerConf{
+					DisableMetricsExpiration: tt.disableMetricsExpiration,
+				}
+				ps := &PromSurfacer{
+					c:                conf,
+					includeTimestamp: includeTimestamp,
+				}
+				assert.Equal(t, tt.want[i], ps.shouldDisableMetricsExpiration())
+			})
+		}
+	}
+}
+
+func TestIncludeTimestamp(t *testing.T) {
+	tests := []struct {
+		name           string
+		setFlag        bool
+		flagValue      string
+		configValue    *bool
+		expectedResult defaultBoolEnum
+	}{
 		{
-			name:                     "DisableMetricsExpiration true, IncludeTimestamp false",
-			includeTimestamp:         false,
-			disableMetricsExpiration: proto.Bool(true),
-			want:                     true,
+			name:           "flag_true",
+			setFlag:        true,
+			flagValue:      "true",
+			expectedResult: explicitTrue,
 		},
 		{
-			name:                     "DisableMetricsExpiration false, IncludeTimestamp false",
-			includeTimestamp:         false,
-			disableMetricsExpiration: proto.Bool(false),
-			want:                     false,
+			name:           "flag_false",
+			setFlag:        true,
+			flagValue:      "false",
+			expectedResult: explicitFalse,
+		},
+		{
+			name:           "config_true",
+			setFlag:        true,
+			flagValue:      "false",
+			configValue:    proto.Bool(true),
+			expectedResult: explicitTrue,
+		},
+		{
+			name:           "config_false",
+			setFlag:        true,
+			flagValue:      "true",
+			configValue:    proto.Bool(false),
+			expectedResult: explicitFalse,
+		},
+		{
+			name:           "default_behavior",
+			setFlag:        false,
+			configValue:    nil,
+			expectedResult: defaultBehavior,
 		},
 	}
 
+	// Save and restore the actual command line flags
+	oldArgs := os.Args
+	oldCommandLine := flag.CommandLine
+	defer func() {
+		os.Args = oldArgs
+		flag.CommandLine = oldCommandLine
+	}()
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			conf := &configpb.SurfacerConf{}
-			if tt.disableMetricsExpiration != nil {
-				conf.DisableMetricsExpiration = tt.disableMetricsExpiration
+			// Reset the command line flags for each test case
+			os.Args = oldArgs
+			flag.CommandLine = flag.NewFlagSet("", flag.ExitOnError)
+
+			// Set up the flag if needed
+			if tt.setFlag {
+				// Need to parse the flag from command line arguments
+				os.Args = append([]string{oldArgs[0]}, "-prometheus_include_timestamp="+tt.flagValue)
+				f := flag.Bool("prometheus_include_timestamp", false, "")
+				flag.Parse()
+				*includeTimestampFlag = *f
 			}
-			ps := &PromSurfacer{
-				c:                conf,
-				includeTimestamp: tt.includeTimestamp,
+
+			// Create a config with the test case's config value
+			conf := &configpb.SurfacerConf{
+				IncludeTimestamp: tt.configValue,
 			}
-			got := ps.disableMetricsExpiration()
-			if got != tt.want {
-				t.Errorf("disableMetricsExpiration() = %v, want %v", got, tt.want)
+
+			// Call the function under test
+			result := shouldIncludeTimestamp(conf)
+
+			// Verify the result
+			if result != tt.expectedResult {
+				t.Errorf("includeTimestamp() = %v, want %v", result, tt.expectedResult)
 			}
 		})
 	}
@@ -437,35 +506,31 @@ func TestNew(t *testing.T) {
 	tests := []struct {
 		name                 string
 		config               *configpb.SurfacerConf
-		includeTimestampFlag bool
 		metricsPrefixFlag    string
 		notInitServeMux      bool
-		wantIncludeTimestamp bool
+		wantIncludeTimestamp defaultBoolEnum
 		wantMetricsPrefix    string
 		wantErr              bool
 	}{
 		{
 			name:                 "Default",
 			config:               nil,
-			includeTimestampFlag: configpb.Default_SurfacerConf_IncludeTimestamp,
 			metricsPrefixFlag:    "",
-			wantIncludeTimestamp: configpb.Default_SurfacerConf_IncludeTimestamp,
+			wantIncludeTimestamp: defaultBehavior,
 			wantMetricsPrefix:    "",
 		},
 		{
 			name:                 "Flags",
 			config:               nil,
-			includeTimestampFlag: true,
 			metricsPrefixFlag:    "cloudprober_f_",
-			wantIncludeTimestamp: true,
+			wantIncludeTimestamp: defaultBehavior,
 			wantMetricsPrefix:    "cloudprober_f_",
 		},
 		{
 			name:                 "Config override",
 			config:               &configpb.SurfacerConf{IncludeTimestamp: proto.Bool(false), MetricsPrefix: proto.String("cloudprober_")},
-			includeTimestampFlag: true,
 			metricsPrefixFlag:    "cloudprober_f_",
-			wantIncludeTimestamp: false,
+			wantIncludeTimestamp: explicitFalse,
 			wantMetricsPrefix:    "cloudprober_",
 		},
 		{
@@ -486,10 +551,8 @@ func TestNew(t *testing.T) {
 			}
 			defer state.SetDefaultHTTPServeMux(oldHTTPMux)
 
-			*includeTimestampFlag = tt.includeTimestampFlag
 			*metricsPrefixFlag = tt.metricsPrefixFlag
 			defer func() {
-				*includeTimestampFlag = configpb.Default_SurfacerConf_IncludeTimestamp
 				*metricsPrefixFlag = ""
 			}()