SignalFx Output by keitwb · Pull Request #6714 · influxdata/telegraf

keitwb · 2019-11-25T14:16:56Z

Add a basic SignalFx output plugin.

This is a continuation of #5204. Closes #923

Downgraded go-logfmt by one minor version to meet compatibility with SignalFx golib.

I can squash all of this into a single commit is preferred but left it separate to show history for now.

Required for all PRs:

[ X ] Signed CLA.
[ X ] Associated README.md updated.
[ X ] Has appropriate unit tests.

codesmith14 · 2019-11-25T14:55:23Z

LGTM

keitwb · 2020-03-09T14:49:00Z

@sjwang90 The go mod tidy command does two different things between go 1.12 and go 1.13, so I can't get the tests to pass between those two versions. Any advice on what to do?

codesmith14 · 2020-03-09T15:04:40Z

LGTM

keitwb · 2020-03-09T15:29:45Z

I need to make the int64 aligned to a 64-bit boundary to get the 386 tests passing.

danielnelson · 2020-03-10T19:01:43Z

The go mod tidy command does two different things between go 1.12 and go 1.13, so I can't get the tests to pass between those two versions. Any advice on what to do?

Should be fixed if you rebase, now we are running go mod tidy only on 1.13.

keitwb · 2020-05-12T17:22:11Z

@danielnelson All tests passed now, although Github still shows some pending that did pass on CircleCI that I manually reran due to unrelated test failures.

danielnelson · 2020-05-14T21:15:50Z

plugins/outputs/signalfx/signalfx.go

+    ## Maximum batch size of datapoints to send to ingest at once
+    batch_size = 1000
+
+	## Maximum number of datapoints or events to buffer in the agent before dropping data
+	max_buffered = 100000


The library used by this PR doesn't integrate very well with Telegraf, as it wants to take over responsibilities that Telegraf typically controls.

For example, batch size and max bufferer are essentially the same as theTelegraf metric_batch_size and metric_buffer_limit options. The Write method should be able to indicate when a write has failed/succeeded so that Telegraf can manage retries and know when collected data has been written.

I think we should consider removing the SignalFX library and writing a simple client using the http package instead.

Ok, I simplified the plugin to synchronously send using our low-level client library. Removed the now unneeded config in the plugin.

@danielnelson is the way I did it good?

keitwb · 2020-06-03T19:35:55Z

@danielnelson are there any other changes needed?

mcmiv413 · 2020-07-20T19:23:17Z

We are currently a customer of influxdb and I was wondering when this was going to get merged in I know it has been moved in the milestones a few times.

domeger · 2020-07-20T20:24:33Z

@danielnelson +1 for @mcmiv413 comment on needing this for another customer. I have a fairly large influx customer who needs this so they continue on with their internal projects.

plugins/outputs/signalfx/signalfx.go

keitwb · 2020-11-09T20:06:07Z

@ssoroka Is there anything else needed for this PR? Would be good to get it merged soon.

ssoroka · 2020-11-09T20:40:09Z

@keitwb is it possible to remove the signalfx-go library from the dependencies?

keitwb · 2020-11-09T20:48:26Z

@keitwb is it possible to remove the signalfx-go library from the dependencies?

@ssoroka I copy/pasted the small amount of code used from that library into this PR and got rid of the dependency. It was just utility/helper type things for the realm urls.

This output plugin converts the `telegraf.Metrics` into signalfx `datapoint`s and then transmits them to the ingest servers using signalfx golang client lib. As of this commit, the client lib is allowed to pick sane defaults and none of its fields are overridable via telegraf config. This can be changed in the future if needed. The unit tests only test for conversion of `telegraf.Metric`s to the `datapoint` structs. All code that executes after that is assumed to be tested in the signalfx client lib itself (and not worth writing end-to-end tests for). Further enhancements: - Custom ingest urls - Better batching - More extensive tests - Support for events, sent by whitelist only Co-authored-by: Ben Keith <benkeith@splunk.com>

flands · 2020-11-19T00:11:24Z

@ssoroka anything else needed to get this merged?

jadbSFx · 2020-12-07T14:42:27Z

@ssoroka what else is needed to get this merged?

ssoroka

So the only remaining thing is i'm a bit concerned about the dependencies. The signalfx golib seems to import a lot:

github.com/jaegertracing/jaeger is a distributed tracing platform
somehow we ended up importing a javascript interpreter: github.com/gopherjs/gopherjs

keitwb · 2020-12-10T14:38:20Z

It does but none of it is actually used by any of the code here so it would get pruned by the go linker. It isn't very feasible to extract the client from golib without doing major refactoring of it.

jrcamp · 2021-02-16T19:30:04Z

@ssoroka thanks for taking a look. I ran a few tests and the net increase in binary size from this PR is ~1.5MB (most of this comes from github.com/gogo/protobuf/protoc-gen-gogo/descriptor library).

So the only remaining thing is i'm a bit concerned about the dependencies. The signalfx golib seems to import a lot:
* github.com/jaegertracing/jaeger is a distributed tracing platform

Tracing is currently part of the SignalFx client library which uses the Jaegar protobuf definitions. Unfortunately it doesn't look like Jaeger has protobufs as their own consumable which is why this module gets included.

* somehow we ended up importing a javascript interpreter: github.com/gopherjs/gopherjs

This comes from goconvey (testing framework we use). It is only used in *_test.go files and never makes it into the binary. But because tests use it it winds up in go.mod.

Unfortunately reducing the dependencies in golib or extracting the client to a minimal set would require a lot of internal rework. I think we have two options (more welcome if you have other ideas):

Add a dedicated SignalFx client internally to telegraf (would be a forked/reduced version of the existing client). The downside being it is more likely to fall out of date compared to the golib version.
Accept the increase in dependencies. As noted above this increase in dependencies are mitigated to a certain extent.

Would one of those options be acceptable for Telegraf? Would one be better than the other?

Thanks!

…utput

amoghe · 2021-02-25T21:06:14Z

👏 🎉 - thank you everyone for making this happen! Can't wait for the next release (that includes this) to drop!

* [outputs.signalfx] Add output plugin for SignalFX This output plugin converts the `telegraf.Metrics` into signalfx `datapoint`s and then transmits them to the ingest servers using signalfx golang client lib. As of this commit, the client lib is allowed to pick sane defaults and none of its fields are overridable via telegraf config. This can be changed in the future if needed. The unit tests only test for conversion of `telegraf.Metric`s to the `datapoint` structs. All code that executes after that is assumed to be tested in the signalfx client lib itself (and not worth writing end-to-end tests for). Further enhancements: - Custom ingest urls - Better batching - More extensive tests - Support for events, sent by whitelist only Co-authored-by: Ben Keith <benkeith@splunk.com> Co-authored-by: Akshay <akshay.moghe@gmail.com> Co-authored-by: Jay Camp <jcamp@splunk.com>

keitwb changed the title ~~Amoghe signalfx output~~ SignalFx Output Nov 25, 2019

keitwb force-pushed the amoghe_signalfx_output branch 3 times, most recently from 9b3e25b to b374f97 Compare November 25, 2019 14:55

danielnelson added the new plugin label Dec 3, 2019

sjwang90 added this to the 1.14.0 milestone Jan 13, 2020

sjwang90 modified the milestones: 1.14.0, 1.15.0 Feb 14, 2020

keitwb force-pushed the amoghe_signalfx_output branch 3 times, most recently from 35c308a to 250a6fb Compare March 9, 2020 14:48

keitwb force-pushed the amoghe_signalfx_output branch 2 times, most recently from 052b554 to 6ff4ab1 Compare March 9, 2020 18:33

keitwb force-pushed the amoghe_signalfx_output branch from 6ff4ab1 to 05450f8 Compare May 12, 2020 16:48

danielnelson reviewed May 14, 2020

View reviewed changes

keitwb force-pushed the amoghe_signalfx_output branch 2 times, most recently from 9f98634 to ed801e5 Compare May 20, 2020 18:41

danielnelson modified the milestones: 1.15.0, 1.16.0 Jul 7, 2020

darinfisher mentioned this pull request Jul 28, 2020

signalfx output plugin #923

Closed

amoghe reviewed Jul 28, 2020

View reviewed changes

plugins/outputs/signalfx/signalfx.go Outdated Show resolved Hide resolved

plugins/outputs/signalfx/signalfx.go Outdated Show resolved Hide resolved

ssoroka approved these changes Oct 14, 2020

View reviewed changes

sjwang90 modified the milestones: 1.16.0, 1.15.4, Planned Oct 19, 2020

keitwb mentioned this pull request Nov 4, 2020

update telegraf dependency signalfx/signalfx-agent#1512

Closed

keitwb force-pushed the amoghe_signalfx_output branch from 4d1d264 to fcb11f3 Compare November 9, 2020 19:32

keitwb force-pushed the amoghe_signalfx_output branch from fcb11f3 to c79977f Compare November 9, 2020 20:47

keitwb force-pushed the amoghe_signalfx_output branch from c79977f to 51fa20f Compare November 9, 2020 21:47

ssoroka reviewed Dec 7, 2020

View reviewed changes

sjwang90 added the plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins label Jan 26, 2021

sjwang90 removed this from the Planned milestone Jan 29, 2021

ssoroka approved these changes Feb 22, 2021

View reviewed changes

Jay Camp added 2 commits February 22, 2021 18:03

Merge remote-tracking branch 'upstream/master' into amoghe_signalfx_o…

a1259f3

…utput

Document 3rd party licenses

424a7e9

sjwang90 added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 25, 2021

ssoroka merged commit 2cf4b75 into influxdata:master Feb 25, 2021

jrcamp deleted the amoghe_signalfx_output branch February 25, 2021 20:35

Conversation

keitwb commented Nov 25, 2019 • edited by sjwang90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

codesmith14 commented Nov 25, 2019

Uh oh!

keitwb commented Mar 9, 2020

Uh oh!

codesmith14 commented Mar 9, 2020

Uh oh!

keitwb commented Mar 9, 2020

Uh oh!

danielnelson commented Mar 10, 2020

Uh oh!

keitwb commented May 12, 2020

Uh oh!

danielnelson May 14, 2020

Choose a reason for hiding this comment

Uh oh!

keitwb May 20, 2020

Choose a reason for hiding this comment

Uh oh!

keitwb May 22, 2020

Choose a reason for hiding this comment

Uh oh!

keitwb commented Jun 3, 2020

Uh oh!

mcmiv413 commented Jul 20, 2020

Uh oh!

domeger commented Jul 20, 2020

Uh oh!

Uh oh!

Uh oh!

keitwb commented Nov 9, 2020

Uh oh!

ssoroka commented Nov 9, 2020

Uh oh!

keitwb commented Nov 9, 2020

Uh oh!

flands commented Nov 19, 2020

Uh oh!

jadbSFx commented Dec 7, 2020

Uh oh!

ssoroka left a comment

Choose a reason for hiding this comment

Uh oh!

keitwb commented Dec 10, 2020

Uh oh!

jrcamp commented Feb 16, 2021

Uh oh!

amoghe commented Feb 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

keitwb commented Nov 25, 2019 •

edited by sjwang90

Loading