Add a Downsampling writer that drop a percentage of spans

[guanw]

Accidentally removed my forked jaeger project so have to create this new pull request..

Which problem is this PR solving?

• Partially resolves Add a rate limiter to jaeger-ingester #1100

Short description of the changes

• Instead of doing ratelimiting based on the volume, simply adding DownSamplingWriter to wrap other writers so user could specify a float ratio number between 0 ~ 1.0 to drop certain percentage of spans.

Ratio parameter is specified by passing DOWN_SAMPLING_RATIO as env variable.

[guanw]

@yurishkuro I tried running some benchmark against sync.Pool, something like the following two.

Against another version where I just use mutex for locking, it seems the simple mutex beats sync.Pool in both scenarios. I think using sync.Pool here is not necessary since we don't need to do fnv.New64a() everytime hashing: Just need to do fnv.reset() to reset it which doesn't involve memory allocation thus sync.Pool won't help here?(But I might be wrong) So i prefer we just use mutex here.

Ran a benchmark against both mutex and sync.Pool versions that spawns 50000 goroutines where each goroutine WriteSpan 10000 times
(writeSpan is mutex version and writeSpan2 is sync.Pool version)

[black-adder]

mutex looks good.

[yurishkuro]

You should use reset() even with the pool. The point of the pool is to avoid memory allocations.

But in any case, we have an issue with the overall design where we run many go-routines working independently on saving spans, but we have these synchronization points like your mutex that block all writers for little reason. Unfortunately, fixing this requires significant re-design where we create writers per goroutine.

Given that both the explicit mutex and sync.Pool introduce a critical section, the main benefit of the pool should be in avoiding memory allocations while unblocking other goroutines invoking the hash. If you're not getting the benefits of the sync pool with your benchmark, I suspect the benchmark.

[black-adder]

@guanw can you add your benchmarks so we can see? In your benchmark you maybe creating threads but not actually waiting for them to run before the benchmark resolves.

@yurishkuro You _should_ use reset() why?

[black-adder]

If it's some crap about how one day aliens will invade and the underlying implementation will in the future require you to call reset before every call so we should prepare for it now...

[yurishkuro]

@yurishkuro You _should_ use reset() why?

as alternative to allocating a new buffer

[black-adder]

@yurishkuro You _should_ use reset() why?

as alternative to allocating a new buffer

The underlying implementation of the hash is:

func (s *sum64a) Write(data []byte) (int, error) {
	hash := *s
	for _, c := range data {
		hash ^= sum64a(c)
		hash *= prime64
	}
	*s = hash
	return len(data), nil
}

where

type (
	sum64a  uint64
)

The underlying hash function doesn't use a buffer for representation. All reset does is:

func (s *sum64a) Reset()  { *s = 14695981039346656037 }

[guanw]

I tried moving the fnv hash logic directly to our code and get rid of the sync.pool. It reduces the benchmark runtime 5x, I think we could just go with that?

[guanw]

actually ignore my last comment. I'm still going with sync.Pool.

[black-adder]

do we have 100 code coverage?

[black-adder]

nearly there

[black-adder]

nit: space between // and Currently

[black-adder]

also, linter might complain that we are swallowing the error.

[yurishkuro]

Pools should be fields of the factory, not globals. Pointers are not needed.

[guanw]

Not sure I got the first part. Here instance is lowercase so it won't be exposed to other files. In factory.go the default is to disable DownsamplingWriter entirely so doesn't make sense to create pool there if we are not using downsamplingWriter as default?

I will get rid of the pointer though.

[black-adder]

ping

[guanw]

@yurishkuro @black-adder @vprithvi Any more comments on this one?

[guanw]

@yurishkuro any more comments on this?

[guanw]

@yurishkuro updated.

[guanw]

@yurishkuro Please let me know if you have any more comments.