Skip to content

Commit de75bef

Browse files
authored
feat(stovepipe): report the last-green change timestamp from record (#583)
## Summary - Record now emits the creation time of the change the last-green bookmark points at as a `record.last_green_timestamp_seconds` gauge (Unix seconds, tagged by queue), written after the bookmark update is durable. Subtracting it from the current time gives how old the change a queue considers green is. - Adds `SourceControl.ChangeInfo(ctx, uri)` for immutable change metadata (`CreatedAt` as an int64 millisecond timestamp, matching the rest of the repo) plus the fake implementation, and a `metrics.NamedGauge` helper. - Reporting is best-effort: an unresolvable queue, a failed lookup, and a missing timestamp are each counted and logged separately rather than returned, so an observability failure cannot turn a successful record operation into a retry. ## Caveat worth reviewing A tally gauge is reported once per `Update`, not continuously, so this series is sparse by construction: it has a datapoint only when the bookmark advances, and carries no value between advances or after a restart. Queries must aggregate across replicas with `max`/last-value. That means it cannot alert on a queue that *stops* going green — the existing `record.last_green_advanced` counter is the signal for that. The `platform/metrics` README now states this so the next caller of `NamedGauge` sees it. If we want a continuously readable value, the follow-up is a periodic per-queue reporter over `queueconfig.Store.List` + `QueueStore.Get`. ## Test plan - [x] `go build ./...` - [x] `go test ./stovepipe/... ./platform/metrics/...` - [x] `make gazelle` produces no BUILD changes; `gofmt -l` clean - [ ] CI `required-checks` gate - Note: `make mocks` fails at HEAD independently of this branch — mockgen's source mode cannot parse `History`'s generic `page.Page[string]` return, so `sourcecontrol_mock.go` cannot be regenerated locally.
1 parent d9a3cfe commit de75bef

11 files changed

Lines changed: 267 additions & 21 deletions

File tree

platform/metrics/README.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Metrics Utilities (`platform/metrics`)
22

3-
The `metrics` package provides reusable helpers for emitting counters and histograms on a `tally.Scope`.
3+
The `metrics` package provides reusable helpers for emitting counters, gauges, and histograms on a `tally.Scope`.
44

55
## Design
66

@@ -48,6 +48,7 @@ For ad-hoc metrics that do not fit the operation lifecycle:
4848
| Function | Emits | Example |
4949
|----------|-------|---------|
5050
| `NamedCounter(scope, name, counter, value, ...tags)` | `{name}.{counter}` counter | `publish.attempts` |
51+
| `NamedGauge(scope, name, gauge, value, ...tags)` | `{name}.{gauge}` gauge | `record.last_green_timestamp_seconds` |
5152
| `NamedHistogram(scope, name, histogram, buckets, ...tags)` | `{name}.{histogram}` histogram | `process.duration` |
5253

5354
```go
@@ -57,7 +58,9 @@ h := metrics.NamedHistogram(c.scope, "process", "duration", metrics.FastLatencyB
5758
h.RecordDuration(elapsed)
5859
```
5960

60-
Do not emit gauges or timers. Represent operation latency and completion count with lifecycle histograms, and represent instantaneous quantities as sampled histogram values when needed.
61+
Use gauges only for state whose latest value is the whole answer, such as a bookmark timestamp. A gauge is reported once per update rather than continuously, so a gauge set on a discrete event produces a sparse series: it carries no value between updates or after a restart, and each replica reports only the updates it made, so queries must aggregate across replicas with `max` or last-value. State that must be readable at any moment needs a periodic re-emit rather than an event-driven one.
62+
63+
Represent operation latency and completion count with lifecycle histograms; do not emit timers.
6164

6265
### Why histograms, not timers
6366

platform/metrics/metrics.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,11 @@ func NamedHistogram(scope tally.Scope, name string, histogram string, buckets ta
171171
return tagged(scope, tags).SubScope(name).Histogram(histogram, buckets)
172172
}
173173

174+
// NamedGauge sets the {name}.{gauge} gauge to value.
175+
func NamedGauge(scope tally.Scope, name string, gauge string, value float64, tags ...Tag) {
176+
tagged(scope, tags).SubScope(name).Gauge(gauge).Update(value)
177+
}
178+
174179
// tagsToMap converts a slice of Tag to a map for tally.
175180
func tagsToMap(tags []Tag) map[string]string {
176181
m := make(map[string]string, len(tags))

platform/metrics/metrics_test.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,15 @@ func TestNamedHistogram(t *testing.T) {
149149
assert.True(t, ok, "expected process.duration histogram")
150150
}
151151

152+
func TestNamedGauge(t *testing.T) {
153+
scope := tally.NewTestScope("", nil)
154+
NamedGauge(scope, "process", "in_flight", 42, NewTag("queue", "monorepo/main"))
155+
156+
g, ok := scope.Snapshot().Gauges()["process.in_flight+queue=monorepo/main"]
157+
assert.True(t, ok, "expected tagged process.in_flight gauge")
158+
assert.Equal(t, float64(42), g.Value())
159+
}
160+
152161
func TestLatencyBuckets_Sorted(t *testing.T) {
153162
sets := map[string]tally.DurationBuckets{
154163
"FastLatencyBuckets": FastLatencyBuckets,

service/stovepipe/server/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,7 @@ func registerPrimaryControllers(
426426
}
427427
count++
428428

429-
recordController := record.NewController(logger, scope, store, stovepipemq.TopicKeyRecord, "stovepipe-record")
429+
recordController := record.NewController(logger, scope, store, scf, stovepipemq.TopicKeyRecord, "stovepipe-record")
430430
if err := c.Register(recordController); err != nil {
431431
return count, fmt.Errorf("failed to register record controller: %w", err)
432432
}

stovepipe/controller/record/BUILD.bazel

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ go_library(
1111
"//stovepipe/core/loader:go_default_library",
1212
"//stovepipe/core/messagequeue:go_default_library",
1313
"//stovepipe/entity:go_default_library",
14+
"//stovepipe/extension/sourcecontrol:go_default_library",
1415
"//stovepipe/extension/storage:go_default_library",
1516
"@com_github_uber_go_tally//:go_default_library",
1617
"@org_uber_go_zap//:go_default_library",
@@ -26,6 +27,8 @@ go_test(
2627
"//platform/consumer/mock:go_default_library",
2728
"//stovepipe/core/messagequeue:go_default_library",
2829
"//stovepipe/entity:go_default_library",
30+
"//stovepipe/extension/sourcecontrol:go_default_library",
31+
"//stovepipe/extension/sourcecontrol/mock:go_default_library",
2932
"//stovepipe/extension/storage:go_default_library",
3033
"//stovepipe/extension/storage/mock:go_default_library",
3134
"@com_github_stretchr_testify//assert:go_default_library",

stovepipe/controller/record/record.go

Lines changed: 67 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"github.com/uber/submitqueue/stovepipe/core/loader"
3434
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
3535
"github.com/uber/submitqueue/stovepipe/entity"
36+
"github.com/uber/submitqueue/stovepipe/extension/sourcecontrol"
3637
"github.com/uber/submitqueue/stovepipe/extension/storage"
3738
"go.uber.org/zap"
3839
)
@@ -41,11 +42,12 @@ import (
4142
// advances the queue's last-green bookmark when that fact is green. Implements
4243
// consumer.Controller.
4344
type Controller struct {
44-
logger *zap.SugaredLogger
45-
metricsScope tally.Scope
46-
stores storage.Factory
47-
topicKey consumer.TopicKey
48-
consumerGroup string
45+
logger *zap.SugaredLogger
46+
metricsScope tally.Scope
47+
stores storage.Factory
48+
sourceControls sourcecontrol.Factory
49+
topicKey consumer.TopicKey
50+
consumerGroup string
4951
}
5052

5153
// Verify Controller implements consumer.Controller interface at compile time.
@@ -64,15 +66,17 @@ func NewController(
6466
logger *zap.SugaredLogger,
6567
scope tally.Scope,
6668
stores storage.Factory,
69+
sourceControls sourcecontrol.Factory,
6770
topicKey consumer.TopicKey,
6871
consumerGroup string,
6972
) *Controller {
7073
return &Controller{
71-
logger: logger.Named("record_controller"),
72-
metricsScope: scope.SubScope("record_controller"),
73-
stores: stores,
74-
topicKey: topicKey,
75-
consumerGroup: consumerGroup,
74+
logger: logger.Named("record_controller"),
75+
metricsScope: scope.SubScope("record_controller"),
76+
stores: stores,
77+
sourceControls: sourceControls,
78+
topicKey: topicKey,
79+
consumerGroup: consumerGroup,
7680
}
7781
}
7882

@@ -256,10 +260,63 @@ func (c *Controller) advanceLastGreen(ctx context.Context, store storage.Storage
256260
"request_id", request.ID,
257261
"last_green_uri", request.URI,
258262
)
263+
c.emitLastGreenTimestamp(ctx, request)
259264
return nil
260265
}
261266
}
262267

268+
// emitLastGreenTimestamp emits the creation time of the change the bookmark now
269+
// points at, once that bookmark is durable. Reporting is best-effort so an
270+
// observability failure cannot turn a successful record operation into a retry,
271+
// which is why each cause is counted and logged separately instead of returned.
272+
func (c *Controller) emitLastGreenTimestamp(ctx context.Context, request entity.Request) {
273+
queueTag := metrics.NewTag("queue", request.Queue)
274+
275+
sourceControl, err := c.sourceControls.For(sourcecontrol.Config{QueueName: request.Queue})
276+
if err != nil {
277+
metrics.NamedCounter(c.metricsScope, _opName, "last_green_timestamp_resolve_errors", 1, queueTag)
278+
c.logger.Warnw("failed to resolve source control to report the last green timestamp",
279+
"queue", request.Queue,
280+
"error", err,
281+
)
282+
return
283+
}
284+
285+
info, err := sourceControl.ChangeInfo(ctx, request.URI)
286+
if err != nil {
287+
metrics.NamedCounter(c.metricsScope, _opName, "last_green_timestamp_errors", 1, queueTag)
288+
c.logger.Warnw("failed to look up the last green change timestamp",
289+
"queue", request.Queue,
290+
"uri", request.URI,
291+
"error", err,
292+
)
293+
return
294+
}
295+
296+
// SourceControl must report a positive creation timestamp, so a missing one
297+
// is a broken extension contract rather than a lookup failure. Emitting it
298+
// anyway would publish a 1970 timestamp and read as an infinitely stale queue.
299+
if info.CreatedAt <= 0 {
300+
metrics.NamedCounter(c.metricsScope, _opName, "last_green_timestamp_invalid", 1, queueTag)
301+
c.logger.Warnw("source control reported no creation timestamp for the last green change",
302+
"queue", request.Queue,
303+
"uri", request.URI,
304+
"created_at", info.CreatedAt,
305+
)
306+
return
307+
}
308+
309+
// The gauge carries the creation time as Unix seconds, so subtracting it
310+
// from the current time yields the age of the last-green change in seconds.
311+
metrics.NamedGauge(
312+
c.metricsScope,
313+
_opName,
314+
"last_green_timestamp_seconds",
315+
float64(time.UnixMilli(info.CreatedAt).Unix()),
316+
queueTag,
317+
)
318+
}
319+
263320
// isNewerRequest reports whether candidate was ingested after current. An empty
264321
// current means the bookmark has never been set, so any candidate is newer.
265322
func isNewerRequest(queue, candidate, current string) (bool, error) {

stovepipe/controller/record/record_test.go

Lines changed: 110 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"errors"
2020
"testing"
21+
"time"
2122

2223
"github.com/stretchr/testify/assert"
2324
"github.com/stretchr/testify/require"
@@ -26,6 +27,8 @@ import (
2627
consumermock "github.com/uber/submitqueue/platform/consumer/mock"
2728
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
2829
"github.com/uber/submitqueue/stovepipe/entity"
30+
"github.com/uber/submitqueue/stovepipe/extension/sourcecontrol"
31+
sourcecontrolmock "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol/mock"
2932
"github.com/uber/submitqueue/stovepipe/extension/storage"
3033
storagemock "github.com/uber/submitqueue/stovepipe/extension/storage/mock"
3134
"go.uber.org/mock/gomock"
@@ -38,12 +41,16 @@ const (
3841
testURI = "git://remote/monorepo/main/head-sha"
3942
)
4043

44+
var testChangeTime = time.Unix(1_700_000_000, 0).UTC()
45+
4146
// recordMocks bundles the mocks a record controller test case wires
4247
// expectations on.
4348
type recordMocks struct {
44-
reqStore *storagemock.MockRequestStore
45-
queueStore *storagemock.MockQueueStore
46-
factStore *storagemock.MockValidationFactStore
49+
reqStore *storagemock.MockRequestStore
50+
queueStore *storagemock.MockQueueStore
51+
factStore *storagemock.MockValidationFactStore
52+
sourceControl *sourcecontrolmock.MockSourceControl
53+
metricsScope tally.TestScope
4754
}
4855

4956
// expectFactCreated wires a successful fact write and captures it, so a case can
@@ -62,13 +69,31 @@ type staticStorageFactory struct{ store storage.Storage }
6269
// For returns the fixed store aggregate for any queue.
6370
func (f staticStorageFactory) For(storage.Config) (storage.Storage, error) { return f.store, nil }
6471

72+
type staticSourceControlFactory struct {
73+
sourceControl sourcecontrol.SourceControl
74+
}
75+
76+
func (f staticSourceControlFactory) For(sourcecontrol.Config) (sourcecontrol.SourceControl, error) {
77+
return f.sourceControl, nil
78+
}
79+
80+
// failingSourceControlFactory resolves no queue.
81+
type failingSourceControlFactory struct{}
82+
83+
func (failingSourceControlFactory) For(sourcecontrol.Config) (sourcecontrol.SourceControl, error) {
84+
return nil, errors.New("no source control for queue")
85+
}
86+
6587
func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, recordMocks) {
6688
t.Helper()
6789

90+
scope := tally.NewTestScope("", nil)
6891
m := recordMocks{
69-
reqStore: storagemock.NewMockRequestStore(ctrl),
70-
queueStore: storagemock.NewMockQueueStore(ctrl),
71-
factStore: storagemock.NewMockValidationFactStore(ctrl),
92+
reqStore: storagemock.NewMockRequestStore(ctrl),
93+
queueStore: storagemock.NewMockQueueStore(ctrl),
94+
factStore: storagemock.NewMockValidationFactStore(ctrl),
95+
sourceControl: sourcecontrolmock.NewMockSourceControl(ctrl),
96+
metricsScope: scope,
7297
}
7398

7499
store := storagemock.NewMockStorage(ctrl)
@@ -78,8 +103,9 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, recordMo
78103

79104
c := NewController(
80105
zap.NewNop().Sugar(),
81-
tally.NewTestScope("test", nil),
106+
scope,
82107
staticStorageFactory{store: store},
108+
staticSourceControlFactory{sourceControl: m.sourceControl},
83109
stovepipemq.TopicKeyRecord,
84110
"stovepipe-record",
85111
)
@@ -158,11 +184,17 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) {
158184
written = q
159185
return nil
160186
})
187+
m.sourceControl.EXPECT().ChangeInfo(gomock.Any(), testURI).
188+
Return(sourcecontrol.ChangeInfo{CreatedAt: testChangeTime.UnixMilli()}, nil)
161189

162190
require.NoError(t, c.Process(context.Background(), delivery(t, ctrl, recordPayload(t, testID))))
163191
assert.Equal(t, tt.wantURI, written.LastGreenURI)
164192
assert.Equal(t, testID, written.LastGreenRequestID)
165193

194+
gauge, ok := m.metricsScope.Snapshot().Gauges()["record_controller.record.last_green_timestamp_seconds+queue=monorepo/main"]
195+
require.True(t, ok)
196+
assert.Equal(t, float64(testChangeTime.Unix()), gauge.Value())
197+
166198
// The green fact is what authorises the advance.
167199
assert.Equal(t, entity.DegreeGreen, fact.Degree)
168200
assert.Equal(t, testURI, fact.URI)
@@ -173,6 +205,68 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) {
173205
}
174206
}
175207

208+
func TestProcess_TimestampReportingFailureDoesNotFailRecord(t *testing.T) {
209+
tests := []struct {
210+
name string
211+
info sourcecontrol.ChangeInfo
212+
err error
213+
wantCounter string
214+
}{
215+
{
216+
name: "lookup fails",
217+
err: errors.New("boom"),
218+
wantCounter: "record_controller.record.last_green_timestamp_errors+queue=monorepo/main",
219+
},
220+
{
221+
// A zero timestamp breaks the extension contract, so it is counted
222+
// apart from a lookup failure rather than emitted as a 1970 gauge.
223+
name: "timestamp missing",
224+
info: sourcecontrol.ChangeInfo{CreatedAt: 0},
225+
wantCounter: "record_controller.record.last_green_timestamp_invalid+queue=monorepo/main",
226+
},
227+
}
228+
229+
for _, tt := range tests {
230+
t.Run(tt.name, func(t *testing.T) {
231+
ctrl := gomock.NewController(t)
232+
c, m := newController(t, ctrl)
233+
234+
m.reqStore.EXPECT().Get(gomock.Any(), testID).
235+
Return(requestWithState(entity.RequestStateSucceeded), nil)
236+
var fact entity.ValidationFact
237+
m.expectFactCreated(&fact)
238+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(queueRow("", "", 1), nil)
239+
m.queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil)
240+
m.sourceControl.EXPECT().ChangeInfo(gomock.Any(), testURI).Return(tt.info, tt.err)
241+
242+
require.NoError(t, c.Process(context.Background(), delivery(t, ctrl, recordPayload(t, testID))))
243+
assert.Empty(t, m.metricsScope.Snapshot().Gauges())
244+
counter, ok := m.metricsScope.Snapshot().Counters()[tt.wantCounter]
245+
require.True(t, ok)
246+
assert.Equal(t, int64(1), counter.Value())
247+
})
248+
}
249+
}
250+
251+
func TestProcess_UnresolvableSourceControlDoesNotFailRecord(t *testing.T) {
252+
ctrl := gomock.NewController(t)
253+
c, m := newController(t, ctrl)
254+
c.sourceControls = failingSourceControlFactory{}
255+
256+
m.reqStore.EXPECT().Get(gomock.Any(), testID).
257+
Return(requestWithState(entity.RequestStateSucceeded), nil)
258+
var fact entity.ValidationFact
259+
m.expectFactCreated(&fact)
260+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(queueRow("", "", 1), nil)
261+
m.queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil)
262+
263+
require.NoError(t, c.Process(context.Background(), delivery(t, ctrl, recordPayload(t, testID))))
264+
assert.Empty(t, m.metricsScope.Snapshot().Gauges())
265+
counter, ok := m.metricsScope.Snapshot().Counters()["record_controller.record.last_green_timestamp_resolve_errors+queue=monorepo/main"]
266+
require.True(t, ok)
267+
assert.Equal(t, int64(1), counter.Value())
268+
}
269+
176270
func TestProcess_RecordsBrokenFactWithoutAdvancing(t *testing.T) {
177271
ctrl := gomock.NewController(t)
178272
c, m := newController(t, ctrl)
@@ -221,6 +315,8 @@ func TestProcess_AdoptsExistingFactFromSameRequest(t *testing.T) {
221315
if tt.wantUpdate {
222316
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(queueRow("", "", 1), nil)
223317
m.queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil)
318+
m.sourceControl.EXPECT().ChangeInfo(gomock.Any(), testURI).
319+
Return(sourcecontrol.ChangeInfo{CreatedAt: testChangeTime.UnixMilli()}, nil)
224320
}
225321

226322
require.NoError(t, c.Process(context.Background(), delivery(t, ctrl, recordPayload(t, testID))))
@@ -270,6 +366,11 @@ func TestProcess_SkipsBookmarkWhenNotNewer(t *testing.T) {
270366
// No Update: the bookmark only moves forward.
271367

272368
require.NoError(t, c.Process(context.Background(), delivery(t, ctrl, recordPayload(t, testID))))
369+
assert.NotContains(
370+
t,
371+
m.metricsScope.Snapshot().Gauges(),
372+
"record_controller.record.last_green_timestamp_seconds+queue=monorepo/main",
373+
)
273374
})
274375
}
275376
}
@@ -338,6 +439,8 @@ func TestProcess_RetriesBookmarkOnVersionMismatch(t *testing.T) {
338439
m.queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), fresh.Version, fresh.Version+1).
339440
Return(nil),
340441
)
442+
m.sourceControl.EXPECT().ChangeInfo(gomock.Any(), testURI).
443+
Return(sourcecontrol.ChangeInfo{CreatedAt: testChangeTime.UnixMilli()}, nil)
341444

342445
require.NoError(t, c.Process(context.Background(), delivery(t, ctrl, recordPayload(t, testID))))
343446
}

0 commit comments

Comments
 (0)