Skip to content

Commit b08de89

Browse files
ywwgywwg
committed
some cleanup for review
1 parent ac363ce commit b08de89

File tree

3 files changed

+70
-75
lines changed

3 files changed

+70
-75
lines changed

‎prometheus/desc.go

+22-32
Original file line numberDiff line numberDiff line change
@@ -57,8 +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-
// compatID is similar to id, but is a hash of all the relevant names escaped with underscores.
61-
compatID uint64
60+
// escapedID is similar to id, but is a hash of all the metric name escaped
61+
// with underscores.
62+
escapedID uint64
6263
// dimHash is a hash of the label names (preset and variable) and the
6364
// Help string. Each Desc with the same fqName must have the same
6465
// dimHash.
@@ -143,40 +144,19 @@ func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, const
143144
return d
144145
}
145146

146-
// XXX this is gross and will be cleaned up.
147-
d.id, d.dimHash = makeHashes(labelNames, labelValues, help, true)
148-
d.compatID, _ = makeHashes(labelNames, labelValues, help, false)
149-
150-
d.constLabelPairs = make([]*dto.LabelPair, 0, len(constLabels))
151-
for n, v := range constLabels {
152-
d.constLabelPairs = append(d.constLabelPairs, &dto.LabelPair{
153-
Name: proto.String(n),
154-
Value: proto.String(v),
155-
})
156-
}
157-
sort.Sort(internal.LabelPairSorter(d.constLabelPairs))
158-
return d
159-
}
160-
161-
// makeHashes generates hashes for detecting duplicate metrics. It will mutate
162-
// labelNames, escaping them with the Underscore method, if UTF8Collision is
163-
// set to CompatibilityCollision.
164-
func makeHashes(labelNames, labelValues []string, help string, UTF8Collision bool) (id, dimHash uint64) {
165147
xxh := xxhash.New()
148+
escapedXXH := xxhash.New()
166149
for i, val := range labelValues {
167-
if i == 0 && !UTF8Collision {
168-
val = model.EscapeName(val, model.UnderscoreEscaping)
169-
}
170150
xxh.WriteString(val)
171151
xxh.Write(separatorByteSlice)
172-
}
173-
id = xxh.Sum64()
174-
175-
if !UTF8Collision {
176-
for i := range labelNames {
177-
labelNames[i] = model.EscapeName(labelNames[i], model.UnderscoreEscaping)
152+
if i == 0 {
153+
val = model.EscapeName(val, model.UnderscoreEscaping)
178154
}
155+
escapedXXH.WriteString(val)
156+
escapedXXH.Write(separatorByteSlice)
179157
}
158+
d.id = xxh.Sum64()
159+
d.escapedID = escapedXXH.Sum64()
180160

181161
// Sort labelNames so that order doesn't matter for the hash.
182162
sort.Strings(labelNames)
@@ -189,8 +169,18 @@ func makeHashes(labelNames, labelValues []string, help string, UTF8Collision boo
189169
xxh.WriteString(labelName)
190170
xxh.Write(separatorByteSlice)
191171
}
192-
dimHash = xxh.Sum64()
193-
return
172+
173+
d.dimHash = xxh.Sum64()
174+
175+
d.constLabelPairs = make([]*dto.LabelPair, 0, len(constLabels))
176+
for n, v := range constLabels {
177+
d.constLabelPairs = append(d.constLabelPairs, &dto.LabelPair{
178+
Name: proto.String(n),
179+
Value: proto.String(v),
180+
})
181+
}
182+
sort.Sort(internal.LabelPairSorter(d.constLabelPairs))
183+
return d
194184
}
195185

196186
// NewInvalidDesc returns an invalid descriptor, i.e. a descriptor with the

‎prometheus/registry.go

+45-40
Original file line numberDiff line numberDiff line change
@@ -66,16 +66,20 @@ func init() {
6666
// pre-registered.
6767
func NewRegistry() *Registry {
6868
return &Registry{
69-
collectorsByID: map[uint64]Collector{},
70-
collectorsByCompatID: map[uint64]Collector{},
71-
descIDs: map[uint64]struct{}{},
72-
compatDescIDs: map[uint64]struct{}{},
73-
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{},
7474
}
7575
}
7676

77-
func (r *Registry) AllowCompatCollisions(allow bool) *Registry {
78-
r.utf8Collision = allow
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
7983
return r
8084
}
8185

@@ -267,27 +271,28 @@ func (errs MultiError) MaybeUnwrap() error {
267271
type Registry struct {
268272
mtx sync.RWMutex
269273
collectorsByID map[uint64]Collector // ID is a hash of the descIDs.
270-
// stores colletors by compatid, only if compat id is different (otherwise we
271-
// can just do the lookup in the regular map).
272-
collectorsByCompatID map[uint64]Collector
273-
descIDs map[uint64]struct{}
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
277+
descIDs map[uint64]struct{}
274278
// desc ids, only if different
275-
compatDescIDs map[uint64]struct{}
279+
escapedDescIDs map[uint64]struct{}
276280
dimHashesByName map[string]uint64
277281
uncheckedCollectors []Collector
278282
pedanticChecksEnabled bool
279-
utf8Collision bool
283+
// This flag is inverted so that the default can be the false value.
284+
disableLegacyCollision bool
280285
}
281286

282287
// Register implements Registerer.
283288
func (r *Registry) Register(c Collector) error {
284289
var (
285290
descChan = make(chan *Desc, capDescChan)
286291
newDescIDs = map[uint64]struct{}{}
287-
newCompatIDs = map[uint64]struct{}{}
292+
newEscapedIDs = map[uint64]struct{}{}
288293
newDimHashesByName = map[string]uint64{}
289294
collectorID uint64 // All desc IDs XOR'd together.
290-
compatID uint64
295+
escapedID uint64
291296
duplicateDescErr error
292297
)
293298
go func() {
@@ -324,19 +329,19 @@ func (r *Registry) Register(c Collector) error {
324329

325330
// Unless we are in pure UTF-8 mode, also check to see if the descID is
326331
// unique when all the names are escaped to underscores.
327-
if !r.utf8Collision {
328-
if _, exists := r.compatDescIDs[desc.compatID]; exists {
332+
if !r.disableLegacyCollision {
333+
if _, exists := r.escapedDescIDs[desc.escapedID]; exists {
329334
duplicateDescErr = fmt.Errorf("descriptor %s will collide with an existing descriptor when escaped for compatibility with non-UTF8 systems", desc)
330335
}
331-
if _, exists := r.descIDs[desc.compatID]; exists {
336+
if _, exists := r.descIDs[desc.escapedID]; exists {
332337
duplicateDescErr = fmt.Errorf("descriptor %s will collide with an existing descriptor when escaped for compatibility with non-UTF8 systems", desc)
333338
}
334339
}
335-
if _, exists := newCompatIDs[desc.compatID]; !exists {
336-
if desc.compatID != desc.id {
337-
newCompatIDs[desc.compatID] = struct{}{}
340+
if _, exists := newEscapedIDs[desc.escapedID]; !exists {
341+
if desc.escapedID != desc.id {
342+
newEscapedIDs[desc.escapedID] = struct{}{}
338343
}
339-
compatID ^= desc.compatID
344+
escapedID ^= desc.escapedID
340345
}
341346

342347
// Are all the label names and the help string consistent with
@@ -367,10 +372,10 @@ func (r *Registry) Register(c Collector) error {
367372
existing, collision := r.collectorsByID[collectorID]
368373
// Unless we are in pure UTF-8 mode, we also need to check that the
369374
// underscore-escaped versions of the IDs don't match.
370-
if !collision && !r.utf8Collision {
371-
existing, collision = r.collectorsByID[compatID]
375+
if !collision && !r.disableLegacyCollision {
376+
existing, collision = r.collectorsByID[escapedID]
372377
if !collision {
373-
existing, collision = r.collectorsByCompatID[compatID]
378+
existing, collision = r.collectorsByEscapedID[escapedID]
374379
}
375380
}
376381

@@ -396,30 +401,30 @@ func (r *Registry) Register(c Collector) error {
396401

397402
// Only after all tests have passed, actually register.
398403
r.collectorsByID[collectorID] = c
399-
// We only need to store the compatID if it doesn't match the unescaped one.
400-
if compatID != collectorID {
401-
r.collectorsByCompatID[compatID] = 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
402407
}
403408
for hash := range newDescIDs {
404409
r.descIDs[hash] = struct{}{}
405410
}
406411
for name, dimHash := range newDimHashesByName {
407412
r.dimHashesByName[name] = dimHash
408413
}
409-
for hash := range newCompatIDs {
410-
r.compatDescIDs[hash] = struct{}{}
414+
for hash := range newEscapedIDs {
415+
r.escapedDescIDs[hash] = struct{}{}
411416
}
412417
return nil
413418
}
414419

415420
// Unregister implements Registerer.
416421
func (r *Registry) Unregister(c Collector) bool {
417422
var (
418-
descChan = make(chan *Desc, capDescChan)
419-
descIDs = map[uint64]struct{}{}
420-
compatDescIDs = map[uint64]struct{}{}
421-
collectorID uint64 // All desc IDs XOR'd together.
422-
compatID uint64
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
423428
)
424429
go func() {
425430
c.Describe(descChan)
@@ -429,8 +434,8 @@ func (r *Registry) Unregister(c Collector) bool {
429434
if _, exists := descIDs[desc.id]; !exists {
430435
collectorID ^= desc.id
431436
descIDs[desc.id] = struct{}{}
432-
compatID ^= desc.compatID
433-
compatDescIDs[desc.compatID] = struct{}{}
437+
collectorEscapedID ^= desc.escapedID
438+
escpaedDescIDs[desc.escapedID] = struct{}{}
434439
}
435440
}
436441

@@ -445,12 +450,12 @@ func (r *Registry) Unregister(c Collector) bool {
445450
defer r.mtx.Unlock()
446451

447452
delete(r.collectorsByID, collectorID)
448-
delete(r.collectorsByCompatID, compatID)
453+
delete(r.collectorsByEscapedID, collectorEscapedID)
449454
for id := range descIDs {
450455
delete(r.descIDs, id)
451456
}
452-
for id := range compatDescIDs {
453-
delete(r.compatDescIDs, id)
457+
for id := range escpaedDescIDs {
458+
delete(r.escapedDescIDs, id)
454459
}
455460
// dimHashesByName is left untouched as those must be consistent
456461
// throughout the lifetime of a program.

‎prometheus/registry_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -1340,9 +1340,9 @@ func TestAlreadyRegisteredEscapingCollision(t *testing.T) {
13401340
t.Run(tc.name, func(t *testing.T) {
13411341
reg := prometheus.NewRegistry()
13421342
if tc.postInitFlagFlip {
1343-
reg.AllowCompatCollisions(false)
1343+
reg.AllowEscapedCollisions(false)
13441344
} else {
1345-
reg.AllowCompatCollisions(tc.utf8Collision)
1345+
reg.AllowEscapedCollisions(tc.utf8Collision)
13461346
}
13471347
fmt.Println("------------")
13481348
err := reg.Register(tc.counterA())
@@ -1351,7 +1351,7 @@ func TestAlreadyRegisteredEscapingCollision(t *testing.T) {
13511351
}
13521352
// model.NameValidationScheme = model.UTF8Validation
13531353
if tc.postInitFlagFlip {
1354-
reg.AllowCompatCollisions(false)
1354+
reg.AllowEscapedCollisions(false)
13551355
}
13561356
err = reg.Register(tc.counterB())
13571357
if !tc.expectErr {

0 commit comments

Comments
 (0)