Skip to content

Commit bc84057

Browse files
fix(serverless-init): attach per-instance tag to MicroVM enhanced usage metric
Codex flagged on PR #53093 (review comment #53093 (comment)) that MicroVM's enhanced usage metric (aws.lambda.microvm.instance) is emitted with the same static tag set for every MicroVM booted from a given image. MicroVM.GetEnhancedMetricTags returns Usage tags at startup, before the MicroVM's instance ID is known (it only becomes available once the /run lifecycle hook fires), and nothing ever adds it afterward. Under normal autoscaling, with multiple concurrent MicroVMs from the same image, their usage samples become indistinguishable from each other. Rather than mutating shared, periodically-read state on ServerlessMetricAgent (which would need a lock/atomic and broke go vet's copylocks check, since CloudService.Shutdown takes ServerlessMetricAgent by value across every cloud service), this attaches the tag at the point of emission instead: - lifecycle.Server already tracks the instance ID race-free (instanceID *atomic.String, captured in handleRun). Added InstanceID(), a nil-safe accessor, so callers outside the lifecycle package can read it. - MicroVM.CurrentUsageMetricTags() turns that into an "instance:<id>" tag, or nil before /run has fired. - enhanced-metrics.Collector gained an optional usageMetricTagsFunc, invoked on every collection tick and passed through the existing AddEnhancedUsageMetric(..., extraTags ...string) parameter — no new shared mutable state required. - main.go duck-types cloudService against a local usageMetricTagProvider interface to wire this hook; every other cloud service doesn't implement it, so usageMetricTagsFunc stays nil and their usage metrics are unaffected. This keeps the change scoped to MicroVM and the (nil-safe, additive) collector plumbing — no other cloud service, ServerlessMetricAgent, or the Shutdown interface needed to change. Unit tests: dda inv test --targets=./cmd/serverless-init/... Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent e6402b9 commit bc84057

9 files changed

Lines changed: 265 additions & 12 deletions

File tree

cmd/serverless-init/cloudservice/microvm.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,19 @@ func (m *MicroVM) GetMetricPrefix() string { return MicroVMPrefix }
116116
// GetUsageMetricSuffix returns the usage metric suffix.
117117
func (m *MicroVM) GetUsageMetricSuffix() string { return MicroVMUsageMetricSuffix }
118118

119+
// CurrentUsageMetricTags returns the dynamic tags to attach to the enhanced
120+
// usage metric on each periodic emission: the per-instance tag, once known
121+
// from /run, or nil before that (see GetEnhancedMetricTags's doc comment on
122+
// why Usage never carries it directly). "instance" matches the tag key
123+
// AppService/CloudRun use for their own per-instance usage tag
124+
// (appservice.go:73, cloudrun.go:156).
125+
func (m *MicroVM) CurrentUsageMetricTags() []string {
126+
if id := m.server.InstanceID(); id != "" {
127+
return []string{"instance:" + id}
128+
}
129+
return nil
130+
}
131+
119132
// GetOrigin returns the origin tag value.
120133
func (m *MicroVM) GetOrigin() string { return MicroVMOrigin }
121134

cmd/serverless-init/cloudservice/microvm_test.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,81 @@ func TestMicroVMGetEnhancedMetricTagsMissingARN(t *testing.T) {
129129
assert.Equal(t, result.Base["resource_id"], result.Usage["resource_id"])
130130
}
131131

132+
// TestMicroVM_CurrentUsageMetricTags_NilServer_ReturnsNil verifies that
133+
// CurrentUsageMetricTags is safe to call before Init (m.server is nil) — the
134+
// enhanced-metrics collector may call it before the lifecycle server exists.
135+
func TestMicroVM_CurrentUsageMetricTags_NilServer_ReturnsNil(t *testing.T) {
136+
m := &MicroVM{}
137+
assert.Nil(t, m.CurrentUsageMetricTags())
138+
}
139+
140+
// TestMicroVM_CurrentUsageMetricTags_BeforeRun_ReturnsNil verifies that no
141+
// instance tag is produced before /run fires, matching GetEnhancedMetricTags'
142+
// documented behavior that instance_id is unknown until then.
143+
func TestMicroVM_CurrentUsageMetricTags_BeforeRun_ReturnsNil(t *testing.T) {
144+
metricAgent := &serverlessMetrics.ServerlessMetricAgent{}
145+
srv := lifecycle.NewServer(
146+
0,
147+
metricAgent, &noopTraceAgent{}, &noopLogsFlusher{},
148+
metricAgent, metricAgent,
149+
(&MicroVM{}).GetSource(),
150+
time.Second,
151+
lifecycle.NewNoopChildHandle(),
152+
nil, // no forwarder
153+
nil, // no heartbeat
154+
)
155+
m := &MicroVM{server: srv}
156+
assert.Nil(t, m.CurrentUsageMetricTags())
157+
}
158+
159+
// TestMicroVM_CurrentUsageMetricTags_AfterRun_ReturnsInstanceTag verifies the
160+
// end-to-end path: once /run has captured the MicroVM instance ID,
161+
// CurrentUsageMetricTags returns the "instance:<id>" tag the enhanced-metrics
162+
// collector attaches to the usage metric on every subsequent tick.
163+
func TestMicroVM_CurrentUsageMetricTags_AfterRun_ReturnsInstanceTag(t *testing.T) {
164+
metricAgent := &serverlessMetrics.ServerlessMetricAgent{}
165+
srv := lifecycle.NewServer(
166+
0,
167+
metricAgent, &noopTraceAgent{}, &noopLogsFlusher{},
168+
metricAgent, metricAgent,
169+
(&MicroVM{}).GetSource(),
170+
time.Second,
171+
lifecycle.NewNoopChildHandle(),
172+
nil, // no forwarder
173+
nil, // no heartbeat
174+
)
175+
l, err := srv.Listen()
176+
require.NoError(t, err)
177+
go srv.Serve(l)
178+
t.Cleanup(func() {
179+
shutCtx, cancel := context.WithTimeout(context.Background(), time.Second)
180+
defer cancel()
181+
_ = srv.Stop(shutCtx)
182+
})
183+
184+
launchPath := "/aws/lambda-microvms/runtime/v1/run"
185+
body := strings.NewReader(`{"microvmId":"vm-abc123"}`)
186+
resp, err := http.Post("http://"+l.Addr().String()+launchPath, "application/json", body)
187+
require.NoError(t, err)
188+
require.NoError(t, resp.Body.Close())
189+
190+
m := &MicroVM{server: srv}
191+
assert.Equal(t, []string{"instance:vm-abc123"}, m.CurrentUsageMetricTags())
192+
}
193+
194+
// TestMicroVM_SatisfiesUsageMetricTagProvider is a compile-time guard: main.go
195+
// duck-types cloudService against an unexported usageMetricTagProvider
196+
// interface with this exact method set to wire the enhanced-metrics
197+
// collector's dynamic tag hook. If CurrentUsageMetricTags' signature ever
198+
// drifts, that type assertion silently stops matching instead of failing to
199+
// compile — this pins the method set so such drift shows up here instead.
200+
func TestMicroVM_SatisfiesUsageMetricTagProvider(t *testing.T) {
201+
var m any = &MicroVM{}
202+
provider, ok := m.(interface{ CurrentUsageMetricTags() []string })
203+
require.True(t, ok, "*MicroVM must implement CurrentUsageMetricTags() []string")
204+
assert.NotPanics(t, func() { provider.CurrentUsageMetricTags() })
205+
}
206+
132207
// Compile-time guard: *MicroVM must satisfy the CloudService interface,
133208
// including the new Run method.
134209
var _ CloudService = (*MicroVM)(nil)

cmd/serverless-init/enhanced-metrics/collector.go

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,15 @@ type Collector struct {
7878
usageMetricSuffix string
7979
// Previous stats for rate calculation
8080
previousRateStats ServerlessRateStats
81+
// usageMetricTagsFunc, when non-nil, is called on every collection tick to
82+
// obtain extra tags for the enhanced usage metric — e.g. MicroVM's
83+
// per-instance tag, which is only known once the /run lifecycle hook
84+
// fires. nil for every cloud service that has no such dynamic tag.
85+
usageMetricTagsFunc func() []string
8186
}
8287

83-
// NewCollector creates a new Collector
84-
func NewCollector(metricAgent EnhancedMetricSender, metricSource metrics.MetricSource, metricPrefix string, usageMetricSuffix string, collectionInterval time.Duration) (*Collector, error) {
88+
// NewCollector creates a new Collector. usageMetricTagsFunc may be nil.
89+
func NewCollector(metricAgent EnhancedMetricSender, metricSource metrics.MetricSource, metricPrefix string, usageMetricSuffix string, collectionInterval time.Duration, usageMetricTagsFunc func() []string) (*Collector, error) {
8590
if metricAgent == nil || reflect.ValueOf(metricAgent).IsNil() {
8691
return nil, errors.New("metricAgent cannot be nil")
8792
}
@@ -92,13 +97,14 @@ func NewCollector(metricAgent EnhancedMetricSender, metricSource metrics.MetricS
9297
}
9398

9499
return &Collector{
95-
metricAgent: metricAgent,
96-
metricSource: metricSource,
97-
cgroupReader: cgroupReader,
98-
collectionInterval: collectionInterval,
99-
metricPrefix: metricPrefix + "enhanced.",
100-
usageMetricSuffix: usageMetricSuffix,
101-
previousRateStats: NullServerlessRateStats,
100+
metricAgent: metricAgent,
101+
metricSource: metricSource,
102+
cgroupReader: cgroupReader,
103+
collectionInterval: collectionInterval,
104+
metricPrefix: metricPrefix + "enhanced.",
105+
usageMetricSuffix: usageMetricSuffix,
106+
previousRateStats: NullServerlessRateStats,
107+
usageMetricTagsFunc: usageMetricTagsFunc,
102108
}, nil
103109
}
104110

@@ -151,7 +157,11 @@ func (c *Collector) collect() {
151157

152158
// Always send the usage metric, regardless of cgroup collection success.
153159
if c.usageMetricSuffix != "" {
154-
c.metricAgent.AddEnhancedUsageMetric(c.metricPrefix+c.usageMetricSuffix, 1, c.metricSource, timestamp)
160+
var extraTags []string
161+
if c.usageMetricTagsFunc != nil {
162+
extraTags = c.usageMetricTagsFunc()
163+
}
164+
c.metricAgent.AddEnhancedUsageMetric(c.metricPrefix+c.usageMetricSuffix, 1, c.metricSource, timestamp, extraTags...)
155165
}
156166

157167
if err := c.cgroupReader.RefreshCgroups(0); err != nil {

cmd/serverless-init/enhanced-metrics/collector_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,3 +307,82 @@ func TestCollectorSendsUsageMetricOnCgroupFailure(t *testing.T) {
307307
metrics.MetricSourceGoogleCloudRunEnhanced, mock.Anything, mock.Anything)
308308
mockAgent.AssertNotCalled(t, "AddEnhancedMetric", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
309309
}
310+
311+
// TestCollectorUsageMetricIncludesDynamicTags verifies that when
312+
// usageMetricTagsFunc is set (MicroVM's use case: attaching the per-instance
313+
// tag once known from /run), its return value is forwarded as extraTags on
314+
// every AddEnhancedUsageMetric call.
315+
func TestCollectorUsageMetricIncludesDynamicTags(t *testing.T) {
316+
mockAgent := new(mockEnhancedMetricSender)
317+
mockAgent.On("AddEnhancedUsageMetric", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return()
318+
319+
mockReader := &mockCgroupReader{refreshErr: errors.New("cgroup failure"), version: 1}
320+
321+
c := &Collector{
322+
metricAgent: mockAgent,
323+
metricSource: metrics.MetricSourceAWSMicroVMEnhanced,
324+
cgroupReader: mockReader,
325+
metricPrefix: "aws.lambda.microvm.enhanced.",
326+
usageMetricSuffix: "instance",
327+
previousRateStats: NullServerlessRateStats,
328+
usageMetricTagsFunc: func() []string { return []string{"instance:vm-abc123"} },
329+
}
330+
331+
c.collect()
332+
333+
mockAgent.AssertCalled(t, "AddEnhancedUsageMetric",
334+
"aws.lambda.microvm.enhanced.instance", float64(1),
335+
metrics.MetricSourceAWSMicroVMEnhanced, mock.Anything, []string{"instance:vm-abc123"})
336+
}
337+
338+
// TestCollectorUsageMetricNilTagsFuncSendsNoExtraTags verifies that when
339+
// usageMetricTagsFunc is nil (every cloud service except MicroVM), no extra
340+
// tags are added to the usage metric — pinning today's behavior.
341+
func TestCollectorUsageMetricNilTagsFuncSendsNoExtraTags(t *testing.T) {
342+
mockAgent := new(mockEnhancedMetricSender)
343+
mockAgent.On("AddEnhancedUsageMetric", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return()
344+
345+
mockReader := &mockCgroupReader{refreshErr: errors.New("cgroup failure"), version: 1}
346+
347+
c := &Collector{
348+
metricAgent: mockAgent,
349+
metricSource: metrics.MetricSourceGoogleCloudRunEnhanced,
350+
cgroupReader: mockReader,
351+
metricPrefix: "gcp.run.container.enhanced.",
352+
usageMetricSuffix: "instance",
353+
previousRateStats: NullServerlessRateStats,
354+
}
355+
356+
c.collect()
357+
358+
mockAgent.AssertCalled(t, "AddEnhancedUsageMetric",
359+
"gcp.run.container.enhanced.instance", float64(1),
360+
metrics.MetricSourceGoogleCloudRunEnhanced, mock.Anything, []string(nil))
361+
}
362+
363+
// TestNewCollectorWiresUsageMetricTagsFunc verifies that NewCollector stores
364+
// the provided usageMetricTagsFunc on the returned Collector so collect()
365+
// picks it up on every tick.
366+
func TestNewCollectorWiresUsageMetricTagsFunc(t *testing.T) {
367+
mockAgent := new(mockEnhancedMetricSender)
368+
tagsFunc := func() []string { return []string{"instance:vm-abc123"} }
369+
370+
c, err := NewCollector(mockAgent, metrics.MetricSourceAWSMicroVMEnhanced, "aws.lambda.microvm.", "instance", time.Second, tagsFunc)
371+
372+
assert.NoError(t, err)
373+
if assert.NotNil(t, c.usageMetricTagsFunc) {
374+
assert.Equal(t, []string{"instance:vm-abc123"}, c.usageMetricTagsFunc())
375+
}
376+
}
377+
378+
// TestNewCollectorNilUsageMetricTagsFuncIsAccepted verifies that
379+
// NewCollector accepts a nil usageMetricTagsFunc — the case for every cloud
380+
// service except MicroVM.
381+
func TestNewCollectorNilUsageMetricTagsFuncIsAccepted(t *testing.T) {
382+
mockAgent := new(mockEnhancedMetricSender)
383+
384+
c, err := NewCollector(mockAgent, metrics.MetricSourceGoogleCloudRunEnhanced, "gcp.run.container.", "instance", time.Second, nil)
385+
386+
assert.NoError(t, err)
387+
assert.Nil(t, c.usageMetricTagsFunc)
388+
}

cmd/serverless-init/enhanced-metrics/collector_unsupported.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ type EnhancedMetricSender interface{}
2020

2121
type Collector struct{}
2222

23-
func NewCollector(_ EnhancedMetricSender, _ metrics.MetricSource, _ string, _ string, _ time.Duration) (*Collector, error) {
23+
func NewCollector(_ EnhancedMetricSender, _ metrics.MetricSource, _ string, _ string, _ time.Duration, _ func() []string) (*Collector, error) {
2424
return nil, errors.New("Collector is only supported on Linux")
2525
}
2626

cmd/serverless-init/lifecycle/server.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,18 @@ func (s *Server) Child() *Child {
305305
// Exposed for white-box tests in external packages; not part of the stable API.
306306
func (s *Server) Heartbeat() *Heartbeat { return s.heartbeat }
307307

308+
// InstanceID returns the MicroVM instance ID captured from /run, or "" if
309+
// /run has not fired yet (or the server is nil). Lets callers outside this
310+
// package — e.g. the enhanced-metrics collector — attach the current
311+
// per-instance tag to metrics emitted after Init, without this package
312+
// needing to know about metric-agent internals.
313+
func (s *Server) InstanceID() string {
314+
if s == nil {
315+
return ""
316+
}
317+
return s.instanceID.Load()
318+
}
319+
308320
func (s *Server) handler() http.Handler {
309321
mux := http.NewServeMux()
310322
mux.HandleFunc(postReady, s.handleReady)

cmd/serverless-init/lifecycle/server_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,26 @@ func TestHandleRunParsesInstanceID(t *testing.T) {
119119
assert.Equal(t, "vm-abc123", id, "instance ID must be stored on the server for lifecycle metric tags")
120120
}
121121

122+
// TestInstanceID_EmptyBeforeRun verifies that InstanceID returns "" before
123+
// /run fires, and the captured ID afterward — the accessor the enhanced
124+
// metrics collector uses to attach a per-instance tag to the usage metric.
125+
func TestInstanceID_EmptyBeforeRun(t *testing.T) {
126+
srv, _, _, _, _, _ := newTestServer()
127+
assert.Empty(t, srv.InstanceID(), "InstanceID must be empty before /run fires")
128+
129+
body := strings.NewReader(`{"microvmId":"vm-abc123"}`)
130+
srv.handleRun(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, pathRun, body))
131+
132+
assert.Equal(t, "vm-abc123", srv.InstanceID())
133+
}
134+
135+
// TestInstanceID_NilServer verifies InstanceID is safe to call on a nil
136+
// *Server, mirroring the existing nil-safety of Child().
137+
func TestInstanceID_NilServer(t *testing.T) {
138+
var srv *Server
139+
assert.Empty(t, srv.InstanceID())
140+
}
141+
122142
// TestHandleRunWithForwarderParsesInstanceID verifies that when a forwarder is
123143
// configured, /run still decodes the MicroVM instance ID from the request body
124144
// before delegating to handleWithForwarder. Without the decode-then-restore fix, the

cmd/serverless-init/main.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,14 @@ func run(secretComp secrets.Component, delegatedAuthComp delegatedauth.Component
124124
return err
125125
}
126126

127+
// usageMetricTagProvider is satisfied by cloud services whose enhanced usage
128+
// metric needs a tag that isn't known until after Init — e.g. MicroVM's
129+
// per-instance tag, only available once the /run lifecycle hook fires.
130+
// Ignored (via the type assertion in setup) by every other cloud service.
131+
type usageMetricTagProvider interface {
132+
CurrentUsageMetricTags() []string
133+
}
134+
127135
func setup(secretComp secrets.Component, delegatedAuthComp delegatedauth.Component, _ mode.Conf, tagger tagger.Component, compression logscompression.Component, hostname hostnameinterface.Component) (cloudservice.CloudService, *serverlessInitLog.Config, *cloudservice.TracingContext, *metrics.ServerlessMetricAgent, logsAgent.ServerlessLogsAgent, *enhancedmetrics.Collector, bool) {
128136
tracelog.SetLogger(log.NewWrapper(3))
129137

@@ -235,9 +243,14 @@ func setup(secretComp secrets.Component, delegatedAuthComp delegatedauth.Compone
235243

236244
setupOtlpAgent(metricAgent, tagger)
237245

246+
var usageMetricTagsFunc func() []string
247+
if p, ok := cloudService.(usageMetricTagProvider); ok {
248+
usageMetricTagsFunc = p.CurrentUsageMetricTags
249+
}
250+
238251
var enhancedMetricsCollector *enhancedmetrics.Collector
239252
if enhancedMetricsEnabled {
240-
enhancedMetricsCollector, err = enhancedmetrics.NewCollector(metricAgent, cloudService.GetSource(), cloudService.GetMetricPrefix(), cloudService.GetUsageMetricSuffix(), 3*time.Second)
253+
enhancedMetricsCollector, err = enhancedmetrics.NewCollector(metricAgent, cloudService.GetSource(), cloudService.GetMetricPrefix(), cloudService.GetUsageMetricSuffix(), 3*time.Second, usageMetricTagsFunc)
241254
if err != nil {
242255
log.Warnf("Failed to initialize enhanced metrics collector: %v", err)
243256
} else {

cmd/serverless-init/main_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,37 @@ func TestBaseTraceTagsComputedFromTagConfigTags(t *testing.T) {
190190
assert.NotEmpty(t, baseTraceTags)
191191
}
192192

193+
// TestUsageMetricTagProvider_MicroVMSatisfiesInterface verifies that
194+
// *cloudservice.MicroVM implements usageMetricTagProvider — the interface
195+
// setup() type-asserts cloudService against to wire the enhanced-metrics
196+
// collector's dynamic per-instance usage tag. If CurrentUsageMetricTags'
197+
// signature ever drifts, the assertion in setup() would silently stop
198+
// matching (no compile error) instead of failing loudly; this test is what
199+
// would catch that.
200+
func TestUsageMetricTagProvider_MicroVMSatisfiesInterface(t *testing.T) {
201+
var cloudService cloudservice.CloudService = &cloudservice.MicroVM{}
202+
_, ok := cloudService.(usageMetricTagProvider)
203+
assert.True(t, ok, "*MicroVM must satisfy usageMetricTagProvider so setup() wires its dynamic usage-metric tags")
204+
}
205+
206+
// TestUsageMetricTagProvider_OtherServicesDoNotSatisfy documents that cloud
207+
// services with no dynamic usage-metric tag are intentionally left out of
208+
// usageMetricTagProvider; setup()'s type assertion falls through to a nil
209+
// usageMetricTagsFunc for them, and NewCollector treats nil as "no extra tags".
210+
func TestUsageMetricTagProvider_OtherServicesDoNotSatisfy(t *testing.T) {
211+
services := []cloudservice.CloudService{
212+
&cloudservice.LocalService{},
213+
&cloudservice.AppService{},
214+
&cloudservice.CloudRun{},
215+
&cloudservice.CloudRunJobs{},
216+
&cloudservice.ContainerApp{},
217+
}
218+
for _, svc := range services {
219+
_, ok := svc.(usageMetricTagProvider)
220+
assert.False(t, ok, "%T must not satisfy usageMetricTagProvider — it has no dynamic usage-metric tag", svc)
221+
}
222+
}
223+
193224
// TestSetupOtlpAgentNoPanic ensures setupOtlpAgent does not panic when OTLP is enabled.
194225
func TestSetupOtlpAgentNoPanic(t *testing.T) {
195226
t.Setenv("DD_OTLP_CONFIG_LOGS_ENABLED", "true")

0 commit comments

Comments
 (0)