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

Rewriting Riemann output plugin #1900

Merged
merged 13 commits into from
Jan 27, 2017
Merged

Rewriting Riemann output plugin #1900

merged 13 commits into from
Jan 27, 2017

Conversation

JamesClonk
Copy link
Contributor

@JamesClonk JamesClonk commented Oct 15, 2016

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

Todo:

  • Unit tests
  • Integration tests
  • README.md

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
@sparrc sparrc added this to the 1.2.0 milestone Nov 2, 2016
@sparrc sparrc modified the milestones: 1.2.0, 1.3.0 Jan 10, 2017
@rclayton-the-terrible
Copy link

Any ideas if this will be merged?

@sparrc
Copy link
Contributor

sparrc commented Jan 13, 2017

I will be trying to get it reviewed next week and hopefully will have it merged within a few weeks, then it will first be in an official release for 1.3

Copy link
Contributor

@sparrc sparrc left a comment

Choose a reason for hiding this comment

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

Overall looks very good, just want some clarification on a few points and some small tweaks.

# address = "localhost:5555"
#
# ## Transport protocol to use, either tcp or udp
# transport = "tcp"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you just make the scheme part of the address string?

ie:

  address = "tcp://localhost:5555"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could, but then I'd have to split/parse it to take it apart again anyway, since the Riemann library needs the protocol and address as separate parameters. address is not used anywhere else otherwise. It's not a complicated thing to do, but I don't see the benefit of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

two benefits:

  1. we can use url.Parse to validate the provided URL
  2. it's more consistent with the configuration of other plugins

#
# ## Riemann TTL, floating-point time in seconds.
# ## Defines how long that an event is considered valid for in Riemann
# # ttl = 30.0
Copy link
Contributor

Choose a reason for hiding this comment

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

what is a "Riemann TTL"? Can you link to documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant a Riemann event TTL. It's described here: http://riemann.io/concepts.html
I clarified the description text a bit.

#
# ## Set measurement name as a Riemann attribute,
# ## instead of prepending it to the Riemann service name
# # measurement_as_attribute = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide more details on what this means? can you provide an example? How does using this option affect the separator option?

Copy link
Contributor Author

@JamesClonk JamesClonk Jan 25, 2017

Choose a reason for hiding this comment

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

I tried to clarify the description text a bit. I also added an example Riemann event with this setting to the plugins Readme.md.

"github.com/influxdata/telegraf/plugins/outputs"
)

const deprecationMsg = "I! WARNING: this Riemann output plugin will be deprecated in a future release, see https://github.com/influxdata/telegraf/issues/1878 for more details & discussion."
Copy link
Contributor

Choose a reason for hiding this comment

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

make this an error message, starting with E!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

#
# ## A list of tag keys whose values get sent as Riemann tags.
# ## If empty, all Telegraf tag values will be sent as tags
# # tag_keys = ["telegraf","custom_tag"]
Copy link
Contributor

Choose a reason for hiding this comment

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

weren't we going to send tags as riemann "attributes" instead of as tags? is that what you mean by "Riemann tags"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Telegraf/Influx/Metrics2.0 tags will be sent as Riemann attributes, which are key/value pairs. But Riemann also has a notion of "Riemann tags" which are just single string thingies. (Kinda like in Graphite I guess?)
This option allows setting such additional tags.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fair enough

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
@jagregory
Copy link
Contributor

Pretty keen on this. If you need anything testing I'll be using this immediately :)

@sparrc
Copy link
Contributor

sparrc commented Jan 27, 2017

just wanted to consolidate the transport and address config options into a single option.

@JamesClonk do you have any objections to making that change?

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>
@JamesClonk
Copy link
Contributor Author

@sparrc ok, I changed it into the combined url setting.

@sparrc sparrc merged commit 3fa37a9 into influxdata:master Jan 27, 2017
njwhite pushed a commit to njwhite/telegraf that referenced this pull request Jan 31, 2017
* rename to riemann_legacy

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* initial draft for Riemann output plugin rewrite

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add unit tests

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add option to send string metrics as states

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add integration tests

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add plugin README.md

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* bump riemann library

* clarify settings description

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* update Readme.md with updated description

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add Riemann event examples

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* use full URL for Riemann server address

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

closes influxdata#1878
mlindes pushed a commit to Comcast/telegraf that referenced this pull request Feb 6, 2017
* rename to riemann_legacy

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* initial draft for Riemann output plugin rewrite

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add unit tests

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add option to send string metrics as states

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add integration tests

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add plugin README.md

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* bump riemann library

* clarify settings description

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* update Readme.md with updated description

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add Riemann event examples

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* use full URL for Riemann server address

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

closes influxdata#1878
maxunt pushed a commit that referenced this pull request Jun 26, 2018
* rename to riemann_legacy

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* initial draft for Riemann output plugin rewrite

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add unit tests

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add option to send string metrics as states

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add integration tests

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add plugin README.md

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* bump riemann library

* clarify settings description

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* update Readme.md with updated description

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* add Riemann event examples

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

* use full URL for Riemann server address

Signed-off-by: Fabio Berchtold <fabio.berchtold@swisscom.com>

closes #1878
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants