-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose new Kafka parameters + tweak defaults #170
Expose new Kafka parameters + tweak defaults #170
Conversation
@mariomac @OlivierCazade @stleerh here I just expose the new parameters from the kafka client that Olivier added to FLP, maybe we should expose more, what do you think? There's for instance BatchMaxLen, which impacts on batching within FLP pipeline. I'm not sure if it's interesting to tweak, as these batches are not sent over the network (bigger batches on that won't decrease network usage as far as I can tell). Also, if we decide to expose that, I wonder if we could then tie kafka's |
Also, this is only for the consumer. What about the producer? Any tweaks to expose? |
api/v1alpha1/flowcollector_types.go
Outdated
|
||
//+kubebuilder:default:=100 | ||
// pullQueueCapacity defines the capacity of the internal message queue used in the Kafka consumer client. | ||
PullQueueCapacity int `json:"pullQueueCapacity"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should rather name it ConsumerQueueCapacity
? (that would be more consistent with the other CRD update PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to specify QueueCapacity, BatchSize and LingerMS as a subsection of each component: for the agent and for FLP.
BTW I think LingerMS in only for Producers (the agent, in this case)
api/v1alpha1/flowcollector_types.go
Outdated
|
||
//+kubebuilder:default:=1000000 | ||
// pullMaxBytes indicates to the broker the maximum batch size that the consumer will accept. Default: 1MB. | ||
PullMaxBytes int `json:"pullMaxBytes"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: ConsumerMaxBytes
?
As they mention, it's important to be able to setup the Kafka's |
Apart of these values being configured at the broker side, as currently specified, we'd need to allow specifying these per producer and consumer, as different producer/consumption patterns would require different batchSize/linger to optimize CPU and consumption. |
UPDATE: I reminded that our Golang library, Segmentio's Kafka-Go, doesn't allow setting the linger value. So we will have to play only with the max batch size. |
About linger: it seems indeed to be lacking in segmentio's api, however, is that really an issue since we already have our own batch/timeout implemented in the agent for grouping flows? |
@jotak I think we can just avoid linger configuration. |
47ce607
to
3fc9186
Compare
api/v1alpha1/flowcollector_types.go
Outdated
@@ -215,6 +215,21 @@ type FlowCollectorKafka struct { | |||
// tls client configuration. | |||
// +optional | |||
TLS ClientTLS `json:"tls"` | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move Consumer...
vars to FLP configuration and ProducerBatchSize
to the agent configuration.
If we use Kafka to connect future functionalities, the queue capacities and batch sizes should probably be different for each component, according to their load amount and pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, sounds like a good argument, especially as we plan to allow kafka exports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariomac done
ae5a216
to
4274c62
Compare
|
@@ -53,6 +53,10 @@ func NewConnTrackParams(name string, ct api.ConnTrack) StageParam { | |||
return StageParam{Name: name, Extract: &Extract{Type: api.ConnTrackType, ConnTrack: &ct}} | |||
} | |||
|
|||
func NewTimbasedParams(name string, ct api.ExtractTimebased) StageParam { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misspelling: NewTimebasedParams
@@ -78,6 +83,8 @@ func (cg *ConfGen) GenerateTruncatedConfig() []config.StageParam { | |||
parameters[i] = config.NewTransformNetworkParams("transform_network", *cg.config.Transform.Network) | |||
case "extract_aggregate": | |||
parameters[i] = config.NewAggregateParams("extract_aggregate", cg.aggregateDefinitions) | |||
case "extract_timebased": | |||
parameters[i] = config.NewTimbasedParams("extract_timebased", cg.timebasedTopKs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misspelling: NewTimebasedParams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in vendors, so that's actually in FLP repo
It would be good if there was a generic way to pass any config settings to Kafka as an advanced feature. Then it could expose the more common ones only. |
yeah, I don't know exactly what's the best way to do that, at some point I was wondering if we shouldn't have a second CRD for advanced configuration .. the point being to try keeping things simple for the average user. iirc @OlivierCazade suggested also something like that previously |
- PullQueueCapacity: capacity of the kafka consumer client internal queue - PullMaxBytes: indicates to the broker the maximum batch size that the consumer will accept. Add producerBatchSize, add metric, tweak defaults New metric "bandwidth_per_namespace" allows to get flows sizes (ie. extracted Bytes sum) per source and dest namespace That proves useful for self-monitoring, eg. to get the overhead of netobserv's traffic compared to the rest of the traffic. Also, setting a much higher cacheMaxSize for the ebpf agent, to reduce traffic overhead. Also raising limits for agent+FLP to 800MB
4274c62
to
e2824b5
Compare
PR rebased @mariomac are you thinking exposing that instead of having these new fields in the CR? (hence we would reject this PR?) We can also take a look at how the loki-operator works, seems it also allow overriding some settings, I'm curious if their approach differs |
@jotak I'm not against exposing the values via CRD, but I think that at the mid term it's better to just allow overriding some internals, so we don't need to create a patch and release a new version every time we want to tweak some internals for a given customer. However, allow overriding the FLP configuration might be complex, as it is not just a properties map but a structured and connected graph |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kafka e2e test code cleanup
Follow-up on netobserv/flowlogs-pipeline#309
queue
Update
Add producerBatchSize, add metric, tweak defaults
3fc9186
New metric "bandwidth_per_namespace" allows to get flows sizes (ie.
extracted Bytes sum) per source and dest namespace
That proves useful for self-monitoring, eg. to get the overhead of
netobserv's traffic compared to the rest of the traffic.
Also, setting a much higher cacheMaxSize for the ebpf agent, to reduce
traffic overhead.
Also raising limits for agent+FLP to 800MB
And tweaking loki client default to decrease number of retries and increase batch size