Skip to content
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

Adding kafka forwarder plugins #123

Closed
wants to merge 2 commits into from
Closed

Conversation

Arunkumarlnr
Copy link

@Arunkumarlnr Arunkumarlnr commented Nov 28, 2022

We have added kafka forwarder plugins for event, meters, metrics, profiling, management and jvm.

@wu-sheng
Copy link
Member

Could you provide your use case?

@wu-sheng wu-sheng added the enhancement New feature or request label Nov 28, 2022
@Arunkumarlnr
Copy link
Author

@wu-sheng
We wanted to send all of our data through Kafka from satellite component. Previously we had support for sending only logs now we have added for all others also.

Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, please complete the following steps:

  1. Fix the comments.
  2. Using make gen-docs to generate the documentation.
  3. You need to create an E2E to verify the Kafka forwards are working well.

Comment on lines 59 to 61
new(grpc_nativeclr.Forwarder),
new(grpc_nativeprocess.Forwarder),
new(grpc_nativeebpfprofiling.Forwarder),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you need to delete the existing forwarders?

Comment on lines 39 to 42
// empty struct for set
type void struct{}

var empty void
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 these fields and struct are not used, why do you declare in there?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your contribution, please complete the following steps:

  1. Fix the comments.
  2. Using make gen-docs to generate the documentation.
  3. You need to create an E2E to verify the Kafka forwards are working well.

Sure will check and fix all the comments added..

Comment on lines +90 to +102
var message []*sarama.ProducerMessage
for _, e := range batch {
data := e.GetData().(*v1.SniffData_Meter)
rawdata, ok := proto.Marshal(data.Meter)
if ok != nil {
return ok
}
message = append(message, &sarama.ProducerMessage{
Topic: f.Topic,
Value: sarama.ByteEncoder(rawdata),
})
}
return f.producer.SendMessages(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

The meter data needs to be sent to the consistent backend address. Please follow the native gRPC Meter forwarder to get more information.

Choose a reason for hiding this comment

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

@mrproliu , Can you please elaborate a bit more on this as here we are sending to kafka and and then oap reads from the same topic

Copy link
Member

Choose a reason for hiding this comment

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

Kafka has the partitioning mechanism. Some metrics like meter should always forward to the consistent partition

Choose a reason for hiding this comment

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

Thanks @wu-sheng , We will check this

# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add the files under the target directory.

@wu-sheng
Copy link
Member

Another thing matters, please provide the sizes of binary before and after these new forwarders.
Also, we need a license update.

@wu-sheng
Copy link
Member

@wu-sheng We wanted to send all of our data through Kafka from satellite component. Previously we had support for sending only logs now we have added for all others also.

Log's Kafka forward doesn't mean anything actually. It is a legacy feature. In the very beginning of Satellite, the agent doesn't provide log reporter, and Satellite did instead.

@wu-sheng wu-sheng closed this Mar 14, 2023
@wu-sheng wu-sheng mentioned this pull request Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants