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

Telegraf 2.0 breaking changes #2320

Closed
sparrc opened this issue Jan 25, 2017 · 16 comments
Closed

Telegraf 2.0 breaking changes #2320

sparrc opened this issue Jan 25, 2017 · 16 comments

Comments

@sparrc
Copy link
Contributor

sparrc commented Jan 25, 2017

This issue is meant to track a list of breaking changes that could be made for the Telegraf 2.0 release.

Breaking changes can still be made during the 1.x cycle if it is determined that the existing plugin or metrics are especially poorly designed or have problems with high cardinality. See the Riemann output plugin as one such example.

This is meant to be an exhaustive list and any major changes should be debated in this issue before being added to the list.

Proposed breaking changes thus far:

  1. HAProxy input should send original metric names from CSV output: Haproxy input missing lots of fields #2318
  2. Consistent host/url/address naming/formatting (OpenTSDB output plugin broken when host is given without scheme #2299 (comment))
  3. Consistent server/address/hostname/etc. tag naming (Telegraf 2.0 breaking changes #2320 (comment))
  4. Removal of deprecated/legacy plugins:
    • udp & tcp listeners
    • riemann_legacy
    • snmp_legacy
    • jolokia
    • kafka_consumer_legacy
@phemmer
Copy link
Contributor

phemmer commented Jan 25, 2017

Consistent host/url/address naming/formatting (#2299 (comment))

@sparrc sparrc added this to the 2.0.0 milestone Jan 25, 2017
@lpic10
Copy link
Contributor

lpic10 commented Jan 25, 2017

Maybe try to come up with some generic rules for metric and tag names for the input plugins to improve consistency.

Some are too generic, same tag can mean different things in different plugins (eg. name tag in the diskio plugin, name tag in the rabbitmq plugin);

Some have same meaning, but different names (host/hostname/node/server tags).

@phemmer
Copy link
Contributor

phemmer commented Jan 28, 2017

Some are too generic, same tag can mean different things in different plugins (eg. name tag in the diskio plugin, name tag in the rabbitmq plugin);

I don't see this as a bad thing. The tag has context since each of the given examples is within a different measurement. If all of them were dumped into a single measurement, then yes, it'd be a problem. But a disk_name tag in the disk measurement seems redundant.

@lpic10
Copy link
Contributor

lpic10 commented Jan 28, 2017

I don't see this as a bad thing. The tag has context since each of the given examples is within a different measurement. If all of them were dumped into a single measurement, then yes, it'd be a problem. But a disk_name tag in the disk measurement seems redundant.

Just a small correction, the disk input uses device tag, it is the diskio that tags as name.

Maybe that's not a bad thing overall, it is just this specific case. Most of the tags are somehow descriptive, but name in particular seems too generic.

@phemmer
Copy link
Contributor

phemmer commented Apr 3, 2017

Consistent fielddrop/tagdrop/tagexclude.
Given the naming, one would think that fielddrop and tagdrop do the same thing, just for fields & tags. But they don't. fielddrop removes a specific field, while tagdrop removes the entire point. Instead tagexclude is what does the tag equivalent of fielddrop. I think the naming should be made consistent to prevent confusion.
(ditto for fieldpass/tagpass/taginclude)

As a side note, the current description of tagpass is incorrect. It's documented as behaving like fieldpass does.

@phemmer
Copy link
Contributor

phemmer commented Apr 4, 2017

Nitpick, but in the procstat input, some of the fields are <noun>_<adjective> while other fields are <adjective>_<noun>. For example cpu_time_system/cpu_time_user vs voluntary_context_switches/involunary_context_switches. Naming should be consistent. I personally think <noun>_<adjective> is better, as it groups the related fields together when sorted.

This is probably something that should be applied as a design policy to all telegraf inputs.

@danielnelson
Copy link
Contributor

In general I agree, but it also might be good to stick to the upstream naming.

@danielnelson
Copy link
Contributor

tagpass: tag names and arrays of strings that are used to filter measurements by the current input. Each string in the array is tested as a glob match against the tag name, and if it matches the measurement is emitted.

This seems right to me.

@phemmer
Copy link
Contributor

phemmer commented Apr 5, 2017

It's not. That's the description of how taginclude behaves. Compare the description of tagpass with the description of fieldpass. Notice how they're pretty much the same, even though their behaviors are not.

tagpass takes a map of tag keys, and acceptable values. If the named tag has any of the given values, it is allowed through. The match is against the tag value, not the tag name.

@danielnelson
Copy link
Contributor

danielnelson commented Apr 5, 2017

I think of it like this: taginclude only removse the tag, the measurement is always emitted. tagpass can remove the whole measurement. This seems consistent with the docs to me.

tag names and arrays of strings

This is basically a poor way of saying a map with tags as the keys and []string as the value.

if it matches the measurement is emitted

So the entire point is emitted or dropped. The match is against both the tag name and values.

@phemmer
Copy link
Contributor

phemmer commented Apr 5, 2017

This is basically a poor way of saying a map containing with tags as the keys and []string as the value.

Even if you argue that, this is still wrong:

Each string in the array is tested as a glob match against the tag name

Each string in the array is tested as a glob match against the tag's value.

I think of it like this: taginclude only removse the tag, the measurement is always emitted. tagpass can remove the whole measurement.

I'm arguing that this is confusing because it's not how fieldpass behaves. If fieldpass/fielddrop matches a field name, and keeps/drops that specific field, while still emitting the point. I would expect tagpass/tagdrop matches a tag name, and keeps/drops that specific tag, while still emitting the point. It does not.

We should also not use the term "measurement". The proper term is "point". Just more confusion.

@danielnelson
Copy link
Contributor

Got it about the name vs value thing, I'll make a PR on this and try to clean up the wording.

We should also not use the term "measurement". The proper term is "point".

True, we use this language throughout the docs in Telegraf.

@danielnelson
Copy link
Contributor

In regards to naming conventions, I wonder what our policy should be on including units in a measurement. I'm imagining something like response_timeout vs response_timeout_seconds.

@phemmer
Copy link
Contributor

phemmer commented Apr 13, 2017

That's a sticky question. On the one hand the clarity is nice. On the other hand, that's a long name, and some units might get even longer, such as "foo_bar_requests_per_minute".
I think as long as documentation is clear, it's sufficient. But no strong preference either way.

@danielnelson
Copy link
Contributor

We should drop support for the old config format deprecated in 0.2.3, where you can only have one plugin of each type.

@sspaink
Copy link
Contributor

sspaink commented Jul 27, 2021

Closing in favor of #9478

@sspaink sspaink closed this as completed Jul 27, 2021
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

No branches or pull requests

6 participants