Skip to content

Commit 8e9ef8c

Browse files
ywwgywwg
committed
By default, return an error if metrics collide when escaped to underscores
Signed-off-by: Owen Williams <[email protected]>
1 parent 93c851f commit 8e9ef8c

File tree

3 files changed

+271
-10
lines changed

3 files changed

+271
-10
lines changed

‎prometheus/desc.go

+13-1
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ type Desc struct {
5757
// must be unique among all registered descriptors and can therefore be
5858
// used as an identifier of the descriptor.
5959
id uint64
60+
// escapedID is similar to id, but is a hash of all the metric name escaped
61+
// with underscores.
62+
escapedID uint64
6063
// dimHash is a hash of the label names (preset and variable) and the
6164
// Help string. Each Desc with the same fqName must have the same
6265
// dimHash.
@@ -142,11 +145,19 @@ func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, const
142145
}
143146

144147
xxh := xxhash.New()
145-
for _, val := range labelValues {
148+
escapedXXH := xxhash.New()
149+
for i, val := range labelValues {
146150
xxh.WriteString(val)
147151
xxh.Write(separatorByteSlice)
152+
if i == 0 {
153+
val = model.EscapeName(val, model.UnderscoreEscaping)
154+
}
155+
escapedXXH.WriteString(val)
156+
escapedXXH.Write(separatorByteSlice)
148157
}
149158
d.id = xxh.Sum64()
159+
d.escapedID = escapedXXH.Sum64()
160+
150161
// Sort labelNames so that order doesn't matter for the hash.
151162
sort.Strings(labelNames)
152163
// Now hash together (in this order) the help string and the sorted
@@ -158,6 +169,7 @@ func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, const
158169
xxh.WriteString(labelName)
159170
xxh.Write(separatorByteSlice)
160171
}
172+
161173
d.dimHash = xxh.Sum64()
162174

163175
d.constLabelPairs = make([]*dto.LabelPair, 0, len(constLabels))

‎prometheus/registry.go

+72-9
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,23 @@ func init() {
6666
// pre-registered.
6767
func NewRegistry() *Registry {
6868
return &Registry{
69-
collectorsByID: map[uint64]Collector{},
70-
descIDs: map[uint64]struct{}{},
71-
dimHashesByName: map[string]uint64{},
69+
collectorsByID: map[uint64]Collector{},
70+
collectorsByEscapedID: map[uint64]Collector{},
71+
descIDs: map[uint64]struct{}{},
72+
escapedDescIDs: map[uint64]struct{}{},
73+
dimHashesByName: map[string]uint64{},
7274
}
7375
}
7476

77+
// AllowEscapedCollisions determines whether the Registry should reject
78+
// Collectors that would collide when escaped to underscores for compatibility
79+
// with older systems. You may set this option to Allow if you know your metrics
80+
// will never be scraped by an older system.
81+
func (r *Registry) AllowEscapedCollisions(allow bool) *Registry {
82+
r.disableLegacyCollision = allow
83+
return r
84+
}
85+
7586
// NewPedanticRegistry returns a registry that checks during collection if each
7687
// collected Metric is consistent with its reported Desc, and if the Desc has
7788
// actually been registered with the registry. Unchecked Collectors (those whose
@@ -258,21 +269,30 @@ func (errs MultiError) MaybeUnwrap() error {
258269
// Registry implements Collector to allow it to be used for creating groups of
259270
// metrics. See the Grouping example for how this can be done.
260271
type Registry struct {
261-
mtx sync.RWMutex
262-
collectorsByID map[uint64]Collector // ID is a hash of the descIDs.
272+
mtx sync.RWMutex
273+
collectorsByID map[uint64]Collector // ID is a hash of the descIDs.
274+
// stores colletors by escapedID, only if escaped id is different (otherwise
275+
// we can just do the lookup in the regular map).
276+
collectorsByEscapedID map[uint64]Collector
263277
descIDs map[uint64]struct{}
278+
// desc ids, only if different
279+
escapedDescIDs map[uint64]struct{}
264280
dimHashesByName map[string]uint64
265281
uncheckedCollectors []Collector
266282
pedanticChecksEnabled bool
283+
// This flag is inverted so that the default can be the false value.
284+
disableLegacyCollision bool
267285
}
268286

269287
// Register implements Registerer.
270288
func (r *Registry) Register(c Collector) error {
271289
var (
272290
descChan = make(chan *Desc, capDescChan)
273291
newDescIDs = map[uint64]struct{}{}
292+
newEscapedIDs = map[uint64]struct{}{}
274293
newDimHashesByName = map[string]uint64{}
275294
collectorID uint64 // All desc IDs XOR'd together.
295+
escapedID uint64
276296
duplicateDescErr error
277297
)
278298
go func() {
@@ -307,6 +327,23 @@ func (r *Registry) Register(c Collector) error {
307327
collectorID ^= desc.id
308328
}
309329

330+
// Unless we are in pure UTF-8 mode, also check to see if the descID is
331+
// unique when all the names are escaped to underscores.
332+
if !r.disableLegacyCollision {
333+
if _, exists := r.escapedDescIDs[desc.escapedID]; exists {
334+
duplicateDescErr = fmt.Errorf("descriptor %s will collide with an existing descriptor when escaped for compatibility with non-UTF8 systems", desc)
335+
}
336+
if _, exists := r.descIDs[desc.escapedID]; exists {
337+
duplicateDescErr = fmt.Errorf("descriptor %s will collide with an existing descriptor when escaped for compatibility with non-UTF8 systems", desc)
338+
}
339+
}
340+
if _, exists := newEscapedIDs[desc.escapedID]; !exists {
341+
if desc.escapedID != desc.id {
342+
newEscapedIDs[desc.escapedID] = struct{}{}
343+
}
344+
escapedID ^= desc.escapedID
345+
}
346+
310347
// Are all the label names and the help string consistent with
311348
// previous descriptors of the same name?
312349
// First check existing descriptors...
@@ -331,7 +368,18 @@ func (r *Registry) Register(c Collector) error {
331368
r.uncheckedCollectors = append(r.uncheckedCollectors, c)
332369
return nil
333370
}
334-
if existing, exists := r.collectorsByID[collectorID]; exists {
371+
372+
existing, collision := r.collectorsByID[collectorID]
373+
// Unless we are in pure UTF-8 mode, we also need to check that the
374+
// underscore-escaped versions of the IDs don't match.
375+
if !collision && !r.disableLegacyCollision {
376+
existing, collision = r.collectorsByID[escapedID]
377+
if !collision {
378+
existing, collision = r.collectorsByEscapedID[escapedID]
379+
}
380+
}
381+
382+
if collision {
335383
switch e := existing.(type) {
336384
case *wrappingCollector:
337385
return AlreadyRegisteredError{
@@ -353,21 +401,30 @@ func (r *Registry) Register(c Collector) error {
353401

354402
// Only after all tests have passed, actually register.
355403
r.collectorsByID[collectorID] = c
404+
// We only need to store the escapedID if it doesn't match the unescaped one.
405+
if escapedID != collectorID {
406+
r.collectorsByEscapedID[escapedID] = c
407+
}
356408
for hash := range newDescIDs {
357409
r.descIDs[hash] = struct{}{}
358410
}
359411
for name, dimHash := range newDimHashesByName {
360412
r.dimHashesByName[name] = dimHash
361413
}
414+
for hash := range newEscapedIDs {
415+
r.escapedDescIDs[hash] = struct{}{}
416+
}
362417
return nil
363418
}
364419

365420
// Unregister implements Registerer.
366421
func (r *Registry) Unregister(c Collector) bool {
367422
var (
368-
descChan = make(chan *Desc, capDescChan)
369-
descIDs = map[uint64]struct{}{}
370-
collectorID uint64 // All desc IDs XOR'd together.
423+
descChan = make(chan *Desc, capDescChan)
424+
descIDs = map[uint64]struct{}{}
425+
escpaedDescIDs = map[uint64]struct{}{}
426+
collectorID uint64 // All desc IDs XOR'd together.
427+
collectorEscapedID uint64
371428
)
372429
go func() {
373430
c.Describe(descChan)
@@ -377,6 +434,8 @@ func (r *Registry) Unregister(c Collector) bool {
377434
if _, exists := descIDs[desc.id]; !exists {
378435
collectorID ^= desc.id
379436
descIDs[desc.id] = struct{}{}
437+
collectorEscapedID ^= desc.escapedID
438+
escpaedDescIDs[desc.escapedID] = struct{}{}
380439
}
381440
}
382441

@@ -391,9 +450,13 @@ func (r *Registry) Unregister(c Collector) bool {
391450
defer r.mtx.Unlock()
392451

393452
delete(r.collectorsByID, collectorID)
453+
delete(r.collectorsByEscapedID, collectorEscapedID)
394454
for id := range descIDs {
395455
delete(r.descIDs, id)
396456
}
457+
for id := range escpaedDescIDs {
458+
delete(r.escapedDescIDs, id)
459+
}
397460
// dimHashesByName is left untouched as those must be consistent
398461
// throughout the lifetime of a program.
399462
return true

‎prometheus/registry_test.go

+186
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636

3737
dto "github.com/prometheus/client_model/go"
3838
"github.com/prometheus/common/expfmt"
39+
"github.com/prometheus/common/model"
3940
"google.golang.org/protobuf/proto"
4041
"google.golang.org/protobuf/types/known/timestamppb"
4142
)
@@ -1181,6 +1182,191 @@ func TestAlreadyRegisteredCollision(t *testing.T) {
11811182
}
11821183
}
11831184

1185+
func TestAlreadyRegisteredEscapingCollision(t *testing.T) {
1186+
oldValidation := model.NameValidationScheme
1187+
model.NameValidationScheme = model.UTF8Validation
1188+
defer func() {
1189+
model.NameValidationScheme = oldValidation
1190+
}()
1191+
1192+
tests := []struct {
1193+
name string
1194+
// These are functions because hashes that determine collision are created
1195+
// at metric creation time.
1196+
counterA func() prometheus.Counter
1197+
counterB func() prometheus.Counter
1198+
utf8Collision bool
1199+
expectErr bool
1200+
// Since the collision mode will be Compat on startup, metrics created in
1201+
// init() functions will use that hashing mode. Metrics created *after*
1202+
// startup could be created with a different hashing mode. This bool tests
1203+
// that case.
1204+
postInitFlagFlip bool
1205+
}{
1206+
{
1207+
name: "no metric name collision",
1208+
counterA: func() prometheus.Counter {
1209+
return prometheus.NewCounter(prometheus.CounterOpts{
1210+
Name: "my_counter_a",
1211+
ConstLabels: prometheus.Labels{
1212+
"name": "label",
1213+
"type": "test",
1214+
},
1215+
})
1216+
},
1217+
counterB: func() prometheus.Counter {
1218+
return prometheus.NewCounter(prometheus.CounterOpts{
1219+
Name: "myAcounterAa",
1220+
ConstLabels: prometheus.Labels{
1221+
"name": "label",
1222+
"type": "test",
1223+
},
1224+
})
1225+
},
1226+
},
1227+
{
1228+
name: "compatibility metric name collision",
1229+
counterA: func() prometheus.Counter {
1230+
return prometheus.NewCounter(prometheus.CounterOpts{
1231+
Name: "my_counter_a",
1232+
ConstLabels: prometheus.Labels{
1233+
"name": "label",
1234+
"type": "test",
1235+
},
1236+
})
1237+
},
1238+
counterB: func() prometheus.Counter {
1239+
return prometheus.NewCounter(prometheus.CounterOpts{
1240+
Name: "my.counter.a",
1241+
ConstLabels: prometheus.Labels{
1242+
"name": "label",
1243+
"type": "test",
1244+
},
1245+
})
1246+
},
1247+
expectErr: true,
1248+
},
1249+
{
1250+
name: "no label value collision",
1251+
counterA: func() prometheus.Counter {
1252+
return prometheus.NewCounter(prometheus.CounterOpts{
1253+
Name: "my_counter_a",
1254+
ConstLabels: prometheus.Labels{
1255+
"name": "label.value",
1256+
"type": "test",
1257+
},
1258+
})
1259+
},
1260+
counterB: func() prometheus.Counter {
1261+
return prometheus.NewCounter(prometheus.CounterOpts{
1262+
Name: "my_counter_a",
1263+
ConstLabels: prometheus.Labels{
1264+
"name": "label_value",
1265+
"type": "test",
1266+
},
1267+
})
1268+
},
1269+
},
1270+
{
1271+
name: "compatibility label name collision",
1272+
counterA: func() prometheus.Counter {
1273+
return prometheus.NewCounter(prometheus.CounterOpts{
1274+
Name: "my_counter_a",
1275+
ConstLabels: prometheus.Labels{
1276+
"label.name": "name",
1277+
"type": "test",
1278+
},
1279+
})
1280+
},
1281+
counterB: func() prometheus.Counter {
1282+
return prometheus.NewCounter(prometheus.CounterOpts{
1283+
Name: "my_counter_a",
1284+
ConstLabels: prometheus.Labels{
1285+
"label_name": "name",
1286+
"type": "test",
1287+
},
1288+
})
1289+
},
1290+
expectErr: true,
1291+
},
1292+
{
1293+
name: "no utf8 metric name collision",
1294+
counterA: func() prometheus.Counter {
1295+
return prometheus.NewCounter(prometheus.CounterOpts{
1296+
Name: "my_counter_a",
1297+
ConstLabels: prometheus.Labels{
1298+
"name": "label",
1299+
"type": "test",
1300+
},
1301+
})
1302+
},
1303+
counterB: func() prometheus.Counter {
1304+
return prometheus.NewCounter(prometheus.CounterOpts{
1305+
Name: "my.counter.a",
1306+
ConstLabels: prometheus.Labels{
1307+
"name": "label",
1308+
"type": "test",
1309+
},
1310+
})
1311+
},
1312+
utf8Collision: true,
1313+
},
1314+
{
1315+
name: "post init flag flip, should collide",
1316+
counterA: func() prometheus.Counter {
1317+
return prometheus.NewCounter(prometheus.CounterOpts{
1318+
Name: "my.counter.a",
1319+
ConstLabels: prometheus.Labels{
1320+
"name": "label",
1321+
"type": "test",
1322+
},
1323+
})
1324+
},
1325+
counterB: func() prometheus.Counter {
1326+
return prometheus.NewCounter(prometheus.CounterOpts{
1327+
Name: "my.counter.a",
1328+
ConstLabels: prometheus.Labels{
1329+
"name": "label",
1330+
"type": "test",
1331+
},
1332+
})
1333+
},
1334+
postInitFlagFlip: true,
1335+
expectErr: true,
1336+
},
1337+
}
1338+
1339+
for _, tc := range tests {
1340+
t.Run(tc.name, func(t *testing.T) {
1341+
reg := prometheus.NewRegistry()
1342+
if tc.postInitFlagFlip {
1343+
reg.AllowEscapedCollisions(false)
1344+
} else {
1345+
reg.AllowEscapedCollisions(tc.utf8Collision)
1346+
}
1347+
fmt.Println("------------")
1348+
err := reg.Register(tc.counterA())
1349+
if err != nil {
1350+
t.Errorf("expected no error, got: %v", err)
1351+
}
1352+
// model.NameValidationScheme = model.UTF8Validation
1353+
if tc.postInitFlagFlip {
1354+
reg.AllowEscapedCollisions(false)
1355+
}
1356+
err = reg.Register(tc.counterB())
1357+
if !tc.expectErr {
1358+
if err != nil {
1359+
t.Errorf("expected no error, got %T", err)
1360+
}
1361+
} else {
1362+
if err == nil {
1363+
t.Error("expected AlreadyRegisteredError, got none")
1364+
}
1365+
}
1366+
})
1367+
}
1368+
}
1369+
11841370
type tGatherer struct {
11851371
done bool
11861372
err error

0 commit comments

Comments
 (0)