Skip to content

Commit dd7ca36

Browse files
thomaskapkcll
andauthored
Add WithClientName (#2246)
* Add WithChipClient * chipClient -> c.clientName --------- Co-authored-by: Pavel <177363085+pkcll@users.noreply.github.com>
1 parent 1749e16 commit dd7ca36

2 files changed

Lines changed: 229 additions & 11 deletions

File tree

pkg/chipingress/batch/client.go

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ type Client struct {
7070
batcherDone chan struct{}
7171
started bool
7272
counters sync.Map // map[seqnumKey]*atomic.Uint64 for per-(source,type) seqnum, cleared on Stop()
73+
clientName string
7374

7475
metrics batchClientMetrics
7576

@@ -88,6 +89,7 @@ type batchClientMetrics struct {
8889
maxGRPCReqSizeAttr otelmetric.MeasurementOption
8990
successStatusAttr otelmetric.MeasurementOption
9091
failureStatusAttr otelmetric.MeasurementOption
92+
clientNameAttr otelmetric.MeasurementOption
9193
}
9294

9395
// Opt is a functional option for configuring the batch Client.
@@ -118,7 +120,7 @@ func NewBatchClient(client chipingress.Client, opts ...Opt) (*Client, error) {
118120
}
119121

120122
var err error
121-
c.metrics, err = newBatchClientMetrics()
123+
c.metrics, err = newBatchClientMetrics(c.clientName)
122124
if err != nil {
123125
return nil, err
124126
}
@@ -291,7 +293,7 @@ func (b *Client) sendBatch(ctx context.Context, messages []*messageWithCallback)
291293

292294
splitBatches := splitMessagesByRequestSize(messages, b.effectiveMaxRequestSize, b.transactionEnabled)
293295
if len(splitBatches) > 1 {
294-
b.metrics.batchSplitsTotal.Add(ctx, 1)
296+
b.metrics.batchSplitsTotal.Add(ctx, 1, b.metrics.clientNameAddOpts()...)
295297
}
296298
for _, batchMessages := range splitBatches {
297299
batchReq, batchBytes := newBatchRequest(batchMessages, b.transactionEnabled)
@@ -350,7 +352,7 @@ func (b *Client) completeBatchCallbacksFromResults(messages []*messageWithCallba
350352
"results", len(results),
351353
"messages", len(messages),
352354
)
353-
b.metrics.resultsMismatchTotal.Add(context.Background(), 1)
355+
b.metrics.resultsMismatchTotal.Add(context.Background(), 1, b.metrics.clientNameAddOpts()...)
354356
}
355357

356358
b.callbackWg.Go(func() {
@@ -377,7 +379,7 @@ func (b *Client) completeBatchCallbacksFromResults(messages []*messageWithCallba
377379
"expected", msg.event.Id,
378380
"got", result.EventId,
379381
)
380-
b.metrics.resultsMismatchTotal.Add(context.Background(), 1)
382+
b.metrics.resultsMismatchTotal.Add(context.Background(), 1, b.metrics.clientNameAddOpts()...)
381383
}
382384
if result.Error != nil {
383385
msg.callback(&PublishError{
@@ -511,6 +513,20 @@ func WithLogger(log *zap.SugaredLogger) Opt {
511513
}
512514
}
513515

516+
// Well-known client_name metric label values for batch client metrics.
517+
// Pass to WithClientName when wiring batch.NewBatchClient.
518+
const (
519+
ClientNameBeholder = "beholder"
520+
ClientNameDurableEmitter = "durable_emitter"
521+
)
522+
523+
// WithClientName sets client_name on batch client metrics. Omitted when unset.
524+
func WithClientName(name string) Opt {
525+
return func(c *Client) {
526+
c.clientName = name
527+
}
528+
}
529+
514530
// WithTransactionEnabled sets PublishOptions.transaction_enabled on every
515531
// batch request. The option is always emitted on the wire so client intent
516532
// is explicit in traces/logs; the server treats unset and explicit false
@@ -525,7 +541,14 @@ func WithTransactionEnabled(transactionEnabled bool) Opt {
525541
}
526542
}
527543

528-
func newBatchClientMetrics() (batchClientMetrics, error) {
544+
func batchMetricAttributeSet(clientName string, kvs ...attribute.KeyValue) attribute.Set {
545+
if clientName != "" {
546+
kvs = append(kvs, attribute.String("client_name", clientName))
547+
}
548+
return attribute.NewSet(kvs...)
549+
}
550+
551+
func newBatchClientMetrics(clientName string) (batchClientMetrics, error) {
529552
meter := otel.Meter("chipingress/batch_client")
530553
sendRequestsTotal, err := meter.Int64Counter(
531554
"chip_ingress.batch.send_requests_total",
@@ -590,6 +613,13 @@ func newBatchClientMetrics() (batchClientMetrics, error) {
590613
return batchClientMetrics{}, err
591614
}
592615

616+
var clientNameAttr otelmetric.MeasurementOption
617+
if clientName != "" {
618+
clientNameAttr = otelmetric.WithAttributeSet(attribute.NewSet(
619+
attribute.String("client_name", clientName),
620+
))
621+
}
622+
593623
return batchClientMetrics{
594624
sendRequestsTotal: sendRequestsTotal,
595625
requestSizeMessages: requestSizeMessages,
@@ -598,23 +628,32 @@ func newBatchClientMetrics() (batchClientMetrics, error) {
598628
configInfo: configInfo,
599629
batchSplitsTotal: batchSplitsTotal,
600630
resultsMismatchTotal: resultsMismatchTotal,
601-
successStatusAttr: otelmetric.WithAttributeSet(attribute.NewSet(
631+
clientNameAttr: clientNameAttr,
632+
successStatusAttr: otelmetric.WithAttributeSet(batchMetricAttributeSet(clientName,
602633
attribute.String("status", "success"),
603634
)),
604-
failureStatusAttr: otelmetric.WithAttributeSet(attribute.NewSet(
635+
failureStatusAttr: otelmetric.WithAttributeSet(batchMetricAttributeSet(clientName,
605636
attribute.String("status", "failure"),
606637
)),
607638
}, nil
608639
}
609640

641+
func (m *batchClientMetrics) clientNameAddOpts() []otelmetric.AddOption {
642+
if m.clientNameAttr != nil {
643+
return []otelmetric.AddOption{m.clientNameAttr}
644+
}
645+
return nil
646+
}
647+
610648
func (m *batchClientMetrics) recordConfig(ctx context.Context, c *Client) {
611-
m.batchSizeAttr = otelmetric.WithAttributeSet(attribute.NewSet(
649+
clientName := c.clientName
650+
m.batchSizeAttr = otelmetric.WithAttributeSet(batchMetricAttributeSet(clientName,
612651
attribute.Int("max_batch_size", c.batchSize),
613652
))
614-
m.maxGRPCReqSizeAttr = otelmetric.WithAttributeSet(attribute.NewSet(
653+
m.maxGRPCReqSizeAttr = otelmetric.WithAttributeSet(batchMetricAttributeSet(clientName,
615654
attribute.Int("max_grpc_request_size_bytes", c.maxGRPCRequestSize),
616655
))
617-
m.configInfo.Record(ctx, 1, otelmetric.WithAttributes(
656+
m.configInfo.Record(ctx, 1, otelmetric.WithAttributeSet(batchMetricAttributeSet(clientName,
618657
attribute.Int("max_batch_size", c.batchSize),
619658
attribute.Int("message_buffer_size", cap(c.messageBuffer)),
620659
attribute.Int("max_concurrent_sends", cap(c.maxConcurrentSends)),
@@ -624,7 +663,7 @@ func (m *batchClientMetrics) recordConfig(ctx context.Context, c *Client) {
624663
attribute.Bool("clone_event", c.cloneEvent),
625664
attribute.Bool("transaction_enabled", c.transactionEnabled),
626665
attribute.Int("max_grpc_request_size_bytes", c.maxGRPCRequestSize),
627-
))
666+
)))
628667
}
629668

630669
func (m *batchClientMetrics) recordSend(ctx context.Context, messageCount int, requestBytes int, latency time.Duration, success bool) {

pkg/chipingress/batch/client_test.go

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1566,6 +1566,185 @@ func TestBatchClient_Metrics(t *testing.T) {
15661566
})
15671567
}
15681568

1569+
func TestBatchClient_ClientNameMetricAttribute(t *testing.T) {
1570+
const clientName = "test_client"
1571+
1572+
batchMetricNames := []string{
1573+
"chip_ingress.batch.send_requests_total",
1574+
"chip_ingress.batch.request_size_messages",
1575+
"chip_ingress.batch.request_size_bytes",
1576+
"chip_ingress.batch.request_latency_ms",
1577+
"chip_ingress.batch.config.info",
1578+
"chip_ingress.batch.batch_splits_total",
1579+
"chip_ingress.batch.results_mismatch_total",
1580+
}
1581+
1582+
t.Run("with WithClientName sets client_name on all batch metrics", func(t *testing.T) {
1583+
reader, restore := useTestMeterProvider(t)
1584+
defer restore()
1585+
1586+
events := []*chipingress.CloudEventPb{
1587+
largeTestEvent("chip-client-1"),
1588+
largeTestEvent("chip-client-2"),
1589+
largeTestEvent("chip-client-3"),
1590+
}
1591+
msgs2 := []*messageWithCallback{{event: events[0]}, {event: events[1]}}
1592+
_, maxRequestSize := newBatchRequest(msgs2, false)
1593+
1594+
mockClient := mocks.NewClient(t)
1595+
done := make(chan struct{})
1596+
var mu sync.Mutex
1597+
var publishCount int
1598+
mockClient.
1599+
On("PublishBatch", mock.Anything, mock.Anything).
1600+
Return(&chipingress.PublishResponse{}, nil).
1601+
Run(func(_ mock.Arguments) {
1602+
mu.Lock()
1603+
publishCount++
1604+
if publishCount == 2 {
1605+
close(done)
1606+
}
1607+
mu.Unlock()
1608+
})
1609+
1610+
client, err := NewBatchClient(
1611+
mockClient,
1612+
WithClientName(clientName),
1613+
WithBatchSize(1),
1614+
WithBatchInterval(time.Second),
1615+
WithMessageBuffer(10),
1616+
WithMaxGRPCRequestSize(minMaxGRPCRequestSize),
1617+
)
1618+
require.NoError(t, err)
1619+
client.maxGRPCRequestSize = maxRequestSize
1620+
client.effectiveMaxRequestSize = maxRequestSize
1621+
client.metrics.recordConfig(t.Context(), client)
1622+
1623+
messages := make([]*messageWithCallback, 0, len(events))
1624+
for _, event := range events {
1625+
messages = append(messages, &messageWithCallback{event: event})
1626+
}
1627+
client.sendBatch(t.Context(), messages)
1628+
1629+
select {
1630+
case <-done:
1631+
case <-time.After(time.Second):
1632+
t.Fatal("timeout waiting for split batches")
1633+
}
1634+
1635+
client.completeBatchCallbacksFromResults(
1636+
[]*messageWithCallback{
1637+
{event: &chipingress.CloudEventPb{Id: "m1", Source: "s", Type: "t"}},
1638+
{event: &chipingress.CloudEventPb{Id: "m2", Source: "s", Type: "t"}},
1639+
},
1640+
[]*chipingress.PublishResult{{EventId: "m1"}},
1641+
)
1642+
client.completeBatchCallbacksFromResults(
1643+
[]*messageWithCallback{
1644+
{event: &chipingress.CloudEventPb{Id: "m1", Source: "s", Type: "t"}},
1645+
},
1646+
[]*chipingress.PublishResult{{EventId: "wrong-id"}},
1647+
)
1648+
1649+
rm := collectResourceMetrics(t, reader)
1650+
for _, name := range batchMetricNames {
1651+
metric := mustMetric(t, rm, name)
1652+
assertMetricHasClientName(t, metric, clientName)
1653+
}
1654+
})
1655+
1656+
t.Run("without WithClientName omits client_name", func(t *testing.T) {
1657+
reader, restore := useTestMeterProvider(t)
1658+
defer restore()
1659+
1660+
mockClient := mocks.NewClient(t)
1661+
mockClient.EXPECT().Close().Return(nil).Maybe()
1662+
done := make(chan struct{})
1663+
mockClient.
1664+
On("PublishBatch", mock.Anything, mock.Anything).
1665+
Return(&chipingress.PublishResponse{}, nil).
1666+
Run(func(_ mock.Arguments) { close(done) }).
1667+
Once()
1668+
1669+
client, err := NewBatchClient(
1670+
mockClient,
1671+
WithBatchSize(1),
1672+
WithBatchInterval(time.Second),
1673+
WithMessageBuffer(10),
1674+
)
1675+
require.NoError(t, err)
1676+
client.Start(t.Context())
1677+
1678+
require.NoError(t, client.QueueMessage(&chipingress.CloudEventPb{
1679+
Id: "no-chip-client", Source: "platform", Type: "Test",
1680+
}, nil))
1681+
1682+
select {
1683+
case <-done:
1684+
case <-time.After(time.Second):
1685+
t.Fatal("timeout waiting for PublishBatch")
1686+
}
1687+
client.Stop()
1688+
1689+
rm := collectResourceMetrics(t, reader)
1690+
for _, sm := range rm.ScopeMetrics {
1691+
for _, metric := range sm.Metrics {
1692+
if !strings.HasPrefix(metric.Name, "chip_ingress.batch.") {
1693+
continue
1694+
}
1695+
assertMetricOmitsClientName(t, metric)
1696+
}
1697+
}
1698+
})
1699+
}
1700+
1701+
func assertMetricHasClientName(t *testing.T, metric metricdata.Metrics, clientName string) {
1702+
t.Helper()
1703+
forEachMetricAttrSet(t, metric, func(attrs attribute.Set) {
1704+
assert.True(t, hasStringAttr(attrs, "client_name", clientName),
1705+
"metric %s missing client_name=%q", metric.Name, clientName)
1706+
})
1707+
}
1708+
1709+
func assertMetricOmitsClientName(t *testing.T, metric metricdata.Metrics) {
1710+
t.Helper()
1711+
forEachMetricAttrSet(t, metric, func(attrs attribute.Set) {
1712+
for _, kv := range attrs.ToSlice() {
1713+
assert.NotEqual(t, "client_name", string(kv.Key), "metric %s should not have client_name", metric.Name)
1714+
}
1715+
})
1716+
}
1717+
1718+
func forEachMetricAttrSet(t *testing.T, metric metricdata.Metrics, fn func(attribute.Set)) {
1719+
t.Helper()
1720+
var count int
1721+
record := func(attrs attribute.Set) {
1722+
count++
1723+
fn(attrs)
1724+
}
1725+
switch data := metric.Data.(type) {
1726+
case metricdata.Sum[int64]:
1727+
for _, dp := range data.DataPoints {
1728+
record(dp.Attributes)
1729+
}
1730+
case metricdata.Histogram[int64]:
1731+
for _, dp := range data.DataPoints {
1732+
record(dp.Attributes)
1733+
}
1734+
case metricdata.Histogram[float64]:
1735+
for _, dp := range data.DataPoints {
1736+
record(dp.Attributes)
1737+
}
1738+
case metricdata.Gauge[int64]:
1739+
for _, dp := range data.DataPoints {
1740+
record(dp.Attributes)
1741+
}
1742+
default:
1743+
t.Fatalf("metric %s has unsupported type %T", metric.Name, metric.Data)
1744+
}
1745+
require.NotZero(t, count, "metric %s has no datapoints", metric.Name)
1746+
}
1747+
15691748
func TestSplitMessagesByRequestSize(t *testing.T) {
15701749
t.Run("empty messages returns nil", func(t *testing.T) {
15711750
result := splitMessagesByRequestSize(nil, 1024, false)

0 commit comments

Comments
 (0)