As we always expect bufferSize to be uint anyway, wdyt of changing the signature to take an uint instead of int?

It seems to make that it'd be better to do the casting at the make(chan ... that comes later rather than at the moment that we pass the value to Initialize 🤔

I will make this change - makes sense to me to constrain it with a uint32 and do the cast at the last possible moment.

We already have a way of having the metrics emitted logging to stdout through the --emit-to-logs variable - did you want this logline for the purposes of checking if metrics are still being emitted regardless of the configured emitter, or for the same purposes as --emit-to-logs?

This code is to check regardless of the configured emitter. If I remember correctly only one of the emitters can be configured at a time. So we cannot have --emit-to-logs and InfluxDB emission configured at the same time.

I'm lukewarm on this; it seems like it'll be super noisy but I guess you can always filter it out. It's already at debug level but we have a decent amount of environments with debug logs enabled. Granted, we're used to maintaining filters to strip out the cruft.

If it was just added while you were developing and isn't useful anymore I would prefer we remove it, but if it was super useful in a pinch I'm fine with keeping it. 🤷‍♂️

Thanks for giving us the context on "why 5000"! That's very helpful.

Wdyt of leaving that to the PR though? As one could always trace the 5000 back to this PR, I'd say leaving that in the PR's comments (or even in the commit message) would be enough 🤔 In the codebase, we don't have many other cases where we add references like this.

I will remove the comments in the code and add as a comment here:

influxdata/docs.influxdata.com-ARCHIVE#454
https://docs.influxdata.com/influxdb/v0.13/write_protocols/write_syntax/#write-a-batch-of-points-with-curl
5000 seems to be the batch size recommended by the InfluxDB team

I'm not very sure about making each emission a goroutine 🤔 If I understood correctly, this makes the buffer "infinite" as there's never really going to exist something that throttles the writes. Is this statement right? Maybe I got something wrong 😬

Making it a goroutine ensures that the construction of the batch points and writing to InfluxDB are immediately taken off the line of execution so that the emitter can continue to build up the next batch.

And the emitLoop function in emit.go still ensures that events are passed on in a synchronous way to the InfluxDBEmitter after they have been read from the channel.

So there will only ever be one open batch that will be closed and submitted for final processing when either the size or duration limit is reached. However, there can potentially be a number of closed batches that are in the process of being transformed into batch points or written to InfluxDB.

Does this answer your question? Or am I missing something?

I only added this file as a last resort. As stated in the counterfeiter docs for third party packages, I tried with
//go:generate counterfeiter github.com/influxdata/influxdb1-client/v2/client.Client
and various other permutations of that, but none of them worked.

I'm lukewarm on this; it seems like it'll be super noisy but I guess you can always filter it out. It's already at debug level but we have a decent amount of environments with debug logs enabled. Granted, we're used to maintaining filters to strip out the cruft.

If it was just added while you were developing and isn't useful anymore I would prefer we remove it, but if it was super useful in a pinch I'm fine with keeping it. 🤷‍♂️

This on the other hand seems like it'd be a bit too noisy. 😅 Would you be OK with removing either this one or the above log?

(Also interesting that the above log line will run even with a nil emitter. Not sure if that's useful. I guess you'd notice it's not configured more quickly, but it also means those logs will show up even in places where metrics are undesired.)

Also noisy - wouldn't this mean there are 3 logs emitted for every metric?

Just checking, is this thread-safe? I guess it is because all of the emits come from an emit loop? 🤔

@vito, yes, that is my understanding. The emitLoop will serialize the emission of events to the InfluxDB code. emitLoop -> emitter.Emit -> emitter.SubmitBatch

I guess the idea behind this is to prevent a giant batch from submitting too slowly and causing the queue to fill up. Makes sense, since that was the original problem. But I wonder if batching would help mitigate that in and of itself, since submiting 5k at a time might cause less backpressure than 1 event 5000 times.

Have you observed any issues with this yet at large scale? It seems like the goroutines could potentially pile up due to a slow consumer.

I think @cirocosta had similar concerns on an earlier revision. I'm fine with how it is for now, but in the future we may want to add a max-in-flight or something. I guess at the end of the day slow consumers are hard to avoid, and this is why we've been thinking of standardizing on Prometheus. 😅

@vito We have not had any issues with this, but I understand your concerns. As a final test I would suggest that once we have merged this and built a release candidate, that we will spin up a concourse cluster similar to our production one and perform some load tests to give us a measure of confidence with all the 5.5 code changes.