Conversation
Required for incoming feature using iter.Seq
Next, actual periodic aggregation.
Compress should not expose the internals of what produce request version is being used during compression. Instead, we replace this with compress flags which can be extended in the future if needed. The current only compress flag is disable gzip. This allows the compressor to be used in other areas of the code unrelated to producing. The dst bytes.Buffer is a bit annoying if used outside of produce but eh it's easy enough to just throw a new(bytes.Buffer) at it, if we do not want to worry about the bytes.Buffer pooling. Uint32 flags should hopefully be enough; since the implementation will be in user hands, I don't want to go overboard on some opaque struct with complex behavior.
This adds a code to the few places needed to observe required (and a _few_ optional) client metrics. Some metrics were removed in this commit; the rebalance latency is underspecified, the fetch latency seems to duplicate consumer node request latency, and assigned partitions seems like medium signal and is tedious to work in. Thus, we are left with 12 metrics, 6 of which are required, of the 21 recommended metrics in the KIP. That's a fine start and we can stick with it unless requested.
* If no requested metrics, sleep for the push interval and re-get * Handle (hopefully) new error with KIP-848 * Actually init the metrics so we do not panic immediately!
This commit fixes a few things after actually testing against a Kafka docker-compose that actually has metrics collection enabled. Notably, * All metrics need to be prefixed with `org.apache.kafka.` * Fixes the switch type case match -- we could elide the & above, but for consistency above, we keep it * Fixes the encoding of NumberDataPoint.value.as_int, which is a sfixed64 not an int. I validated the serialization of a serialized payload by writing it to a file and checking it against protoc; after these changes, it deserializes correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This supports 12 of the recommended 21 metrics; all 6 of the required ones.
KIP-1106 is supported by adding a client option,
UserMetricsFn, allowing you to provide user metrics at every metric aggregation interval. Client metrics can be disabled withDisableClientMetrics.UserMetricsFnreturns aniter.Seq[Metric], requiring the minimum Go version for franz-go to be bumped to 1.23.The compression API was simplified (hopefully) a bit by now not leaking an internal implementation detail.
This has been manually tested in two ways,
protoc.This was done with:
I did not test viewing the metrics at the end, since I don't want to further figure out how to set up 3rd party systems. It would be beneficial for Confluent to provide a KIP-714 enabled broker and local server / log message to check correctness, but alas, a lot of KIP-714 leaves it for the implementor to figure out 👍 .
Unit tests are gated on introducing more third party libraries, which I'd like to avoid.
Closes #848.