feat(inputs.firehose): Add new plugin #15988

syedmhashim · 2024-10-07T10:44:05Z

Summary

The firehose input plugin would be used to receive data from AWS Data Firehose.

Checklist

No AI generated code was used in this PR

Related issues

resolves #15870

srebhan

@syedmhashim I do have some comments, mostly on style, so overall this looks quite promising. I urge you to keep the code as simple and less nested as possible to ease review and debugging. And of course some unit-tests would be great!

plugins/inputs/firehose/README.md

plugins/inputs/firehose/firehose.go

syedmhashim · 2024-10-09T06:43:40Z

@syedmhashim I do have some comments, mostly on style, so overall this looks quite promising. I urge you to keep the code as simple and less nested as possible to ease review and debugging. And of course some unit-tests would be great!

Thanks for the review. Will definitely go over the comments and update accordingly. Just FYI, I have been using http_listener_v2 input plugin as a reference to write the code. The unit tests will come soon as well

srebhan · 2024-10-09T07:49:14Z

Yeah we do have some older code we didn't adapt yet but things changed both on the golang side as well as on us being more strict with the way things are done. ;-) No worries, we will figure it out together. :-)

srebhan · 2024-10-10T11:03:03Z

@syedmhashim I'm loosing a bit the track in the discussion above, could you please push an update with the stuff that is clear and then we discuss the unclear parts?

syedmhashim · 2024-10-11T13:25:15Z

@srebhan Hey! Just pushed some changes and commented "done" on the threads that I resolved. Sorry for the delay. I got occupied with my job

srebhan

That looks much better @syedmhashim! Some more cleanups and comments from my side...

plugins/inputs/firehose/README.md

plugins/inputs/firehose/firehose.go

plugins/inputs/firehose/firehose_request.go

syedmhashim · 2024-10-21T09:11:55Z

@srebhan Hi! Pushed some changes and added comments where relevant

srebhan

@syedmhashim thanks for the update. I tried to put more concrete suggestions into my review. Overall, I think we are almost there, the only thing missing are unit-tests with a mocked sender...

plugins/inputs/firehose/firehose.go

plugins/inputs/firehose/firehose_request.go

plugins/inputs/firehose/sample.conf

plugins/inputs/firehose/firehose_request.go

plugins/inputs/firehose/firehose.go

srebhan

Really nice @syedmhashim! Some very small things with the biggest being that log messages should start with a captial letter... The only thing missing are the unit-tests...

plugins/inputs/firehose/firehose.go

syedmhashim · 2024-11-18T09:34:38Z

Hi @srebhan. In addition to the changes that you suggested, I also made some changes to the code(see last commit) that I think would make it more clean and readable. P.S I think it also helps to achieve separation of concern since now all processing of the request is handled by firehose_request.go while firehose.go mostly focus on adding metrics:

Changes to :

firehose_request.go
- Added newRequest method to handle the creation of request object and (gzip +) json decoding of the request body
- Added processRequest method to handle the authentication, validation, base64 decoding and parameter extraction
- I’ve tried to set the response status code mostly in firehose_request.go
firehose.go
- Added handleRequest method. This helps to get rid of the redundant sendResponse code block.

I'd like to hear your thoughts on this

srebhan

@syedmhashim thanks for the update. I do have some smaller comments in the code but nothing big.

Regarding the tests, I suggest that you don't test the request separately but simply use a http client and send data to the plugin. You might also want to implement a general test-case setup similar to what we do in socket listener tests...

If you need help with the tests, please provide some payload examples and invite me to your branch and I'll help you to drive this PR over the finish line...

srebhan · 2024-12-05T09:53:58Z

plugins/inputs/firehose/firehose.go

+	"github.com/influxdata/telegraf"
+	"github.com/influxdata/telegraf/config"
+	"github.com/influxdata/telegraf/internal"
+	tlsint "github.com/influxdata/telegraf/plugins/common/tls"


We no do have an unwritten convention to use

Suggested change

tlsint "github.com/influxdata/telegraf/plugins/common/tls"

common_tls "github.com/influxdata/telegraf/plugins/common/tls"

i.e. prefix packages in plugins/common with common_...

srebhan · 2024-12-05T09:54:59Z

plugins/inputs/firehose/firehose.go

+		f.listener, err = net.Listen("tcp", f.ServiceAddress)
+	}
+	if err != nil {
+		return err


Suggested change

return err

return fmt.Errorf("creating listener failed: %w", err)

srebhan · 2024-12-05T09:57:10Z

plugins/inputs/firehose/firehose.go

+	go func() {
+		if err := f.server.Serve(f.listener); err != nil {
+			if !errors.Is(err, net.ErrClosed) {
+				f.Log.Errorf("Starting server failed: %v", err)


I think this is not only on startup but might also return errors later... So how about

Suggested change

f.Log.Errorf("Starting server failed: %v", err)

f.Log.Errorf("Server failed: %v", err)

srebhan · 2024-12-05T09:57:30Z

plugins/inputs/firehose/firehose.go

+	err := f.server.Shutdown(context.Background())
+	if err != nil {


Suggested change

err := f.server.Shutdown(context.Background())

if err != nil {

if err := f.server.Shutdown(context.Background()); err != nil {

srebhan · 2024-12-05T10:01:59Z

plugins/inputs/firehose/firehose.go

+func (f *Firehose) handleRequest(req *http.Request) (r *request) {
+	var err error
+	if r, err = newRequest(req); err != nil {
+		f.Log.Errorf("Creating request object failed: %v", err)
+		return r
+	}


How about returning the actual error here and do the logging in ServeHTTP? This will save a few lines I think

Suggested change

func (f *Firehose) handleRequest(req *http.Request) (r *request) {

var err error

if r, err = newRequest(req); err != nil {

f.Log.Errorf("Creating request object failed: %v", err)

return r

}

func (f *Firehose) handleRequest(req *http.Request) (*request, error) {

r, err := newRequest(req)

if err != nil {

return fmt.Errorf("creating request failed: %w", err)

}

of course you need to modify the code below and in ServeHTTP...

I tried implementing this but doesn’t help. The line count remains the same as we need to introduce err checking in the ServeHTTP function. I made some other changes which saves us a couple lines though

syedmhashim · 2024-12-05T13:10:52Z

@srebhan Hey. Just sent you an invite! The payload needs to be in the following format with data having the actual b64 encoded text that would be parsed (based on the parser set) after decoding:

{
    "requestId": “test-id",
    "timestamp": 123456789,
    "records": [
        {
          "data": "aGVsbG8gd29ybGQK" // "hello world"
        }
    ]
}

telegraf-tiger · 2024-12-05T13:22:06Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

⚠️ This pull request increases the Telegraf binary size by 1.78 % for linux amd64 (new size: 261.3 MB, nightly size 256.7 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Oct 7, 2024

srebhan changed the title ~~feat(inputs/firehose): add input plugin for AWS Data Firehose~~ feat(inputs.firehose): Add new plugin Oct 8, 2024

telegraf-tiger bot added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Oct 8, 2024

srebhan reviewed Oct 8, 2024

View reviewed changes

srebhan self-assigned this Oct 8, 2024

srebhan reviewed Oct 16, 2024

View reviewed changes

srebhan reviewed Oct 29, 2024

View reviewed changes

syedmhashim force-pushed the master branch from 3241fdc to d6914ba Compare November 5, 2024 11:38

srebhan reviewed Nov 5, 2024

View reviewed changes

syedmhashim force-pushed the master branch from 3d93830 to 3685c03 Compare November 18, 2024 09:31

syedmhashim force-pushed the master branch 2 times, most recently from 0138d46 to 9d69797 Compare December 5, 2024 07:13

srebhan reviewed Dec 5, 2024

View reviewed changes

syedmhashim added 3 commits December 5, 2024 17:54

feat(inputs/firehose): Add input plugin for AWS Data Firehose

cd9667a

feat(inputs/firehose): update firehose plugin

ad130e5

feat(inputs/firehose): Add tests for firehose_request.go

005417a

syedmhashim force-pushed the master branch from 9d69797 to 005417a Compare December 5, 2024 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.firehose): Add new plugin #15988

feat(inputs.firehose): Add new plugin #15988

syedmhashim commented Oct 7, 2024

srebhan left a comment

syedmhashim commented Oct 9, 2024

srebhan commented Oct 9, 2024

srebhan commented Oct 10, 2024

syedmhashim commented Oct 11, 2024

srebhan left a comment

syedmhashim commented Oct 21, 2024

srebhan left a comment

srebhan left a comment

syedmhashim commented Nov 18, 2024

srebhan left a comment

srebhan Dec 5, 2024

syedmhashim Dec 5, 2024

srebhan Dec 5, 2024

syedmhashim Dec 5, 2024

srebhan Dec 5, 2024

syedmhashim Dec 5, 2024

srebhan Dec 5, 2024

syedmhashim Dec 5, 2024

srebhan Dec 5, 2024

syedmhashim Dec 5, 2024

syedmhashim commented Dec 5, 2024

telegraf-tiger bot commented Dec 5, 2024

Artifact URLs

	tlsint "github.com/influxdata/telegraf/plugins/common/tls"
	common_tls "github.com/influxdata/telegraf/plugins/common/tls"

	return err
	return fmt.Errorf("creating listener failed: %w", err)

	f.Log.Errorf("Starting server failed: %v", err)
	f.Log.Errorf("Server failed: %v", err)

		err := f.server.Shutdown(context.Background())
		if err != nil {

	err := f.server.Shutdown(context.Background())
	if err != nil {
	if err := f.server.Shutdown(context.Background()); err != nil {

feat(inputs.firehose): Add new plugin #15988

Are you sure you want to change the base?

feat(inputs.firehose): Add new plugin #15988

Conversation

syedmhashim commented Oct 7, 2024

Summary

Checklist

Related issues

srebhan left a comment

Choose a reason for hiding this comment

syedmhashim commented Oct 9, 2024

srebhan commented Oct 9, 2024

srebhan commented Oct 10, 2024

syedmhashim commented Oct 11, 2024

srebhan left a comment

Choose a reason for hiding this comment

syedmhashim commented Oct 21, 2024

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

syedmhashim commented Nov 18, 2024

srebhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syedmhashim commented Dec 5, 2024

telegraf-tiger bot commented Dec 5, 2024

Artifact URLs