Skip to content

stats: escape map keys containing dot (.)#2992

Merged
sougou merged 1 commit intovitessio:masterfrom
nerdatmath:master
Jul 15, 2017
Merged

stats: escape map keys containing dot (.)#2992
sougou merged 1 commit intovitessio:masterfrom
nerdatmath:master

Conversation

@nerdatmath
Copy link
Contributor

No description provided.

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we run into a use case that required names to have dots? If not, I think we should avoid this change for the sake of simplicity and performance.

return t
}

var escaper = strings.NewReplacer(".", "\\.", "\\", "\\\\")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is going to be very expensive. I believe these functions get called on every query. Can you benchmark the before and after?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a benchmark for MultiCounters:

var benchMultiCounter = NewMultiCounters("benchMulti", []string{"call", "keyspace", "dbtype")

func BenchmarkMultiCounters(b *testing.B) {
	clear()
        key := []string{"execute-key-ranges", "keyspacename", "replica"}
	benchCounter.Add(key, 1)
	b.ResetTimer()

	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			benchCounter.Add(key, 1)
		}
	})
}

before:
BenchmarkMultiCounters-3 300000000 186 ns/op

after:
BenchmarkMultiCounters-3 200000000 261 ns/op

So this adds only 75 ns to a 186 ns operation (+40%) in the normal case where there are no dots or backslashes in the key. Even if it ran 100 times per query it would only add 7.5 microseconds. Note that the whole key is already being copied by strings.Join; this is only adding a pass to scan the key for 2 characters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I've seen regexp to be more expensive. I guess it's driven by complexity.

@sougou
Copy link
Contributor

sougou commented Jul 15, 2017

LGTM

Approved with PullApprove

@sougou sougou merged commit d1705d1 into vitessio:master Jul 15, 2017
@bbeaudreault
Copy link
Contributor

bbeaudreault commented Jul 18, 2017

This creates invalid json. I believe you need to doubly escape them, that is escape the \ as well: \\. instead of \. Here's a go playground link which shows it failing to be parsed using go encodings/json. It also fails to parse in python:

https://play.golang.org/p/yjseLuMwc7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants