snmp: Don't drop lines with an empty tag #1927

hahnjo · 2016-10-23T13:39:19Z

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)

The changes in #1848 resulted in lines being dropped if they had an empty tag. So let's count empty tags as valid to take the right decision!

sparrc · 2016-10-24T09:13:50Z

I don't think this is exactly the right solution, you should probably add the line but drop the empty tag.

@phemmer please also review

hahnjo · 2016-10-24T12:29:18Z

What does InfluxDB say to lines with less tags? I thought the Telegraf code was kind of ensuring InfluxDB's requirements are met...

If that's not the case, I'll happily rework the patch 👍

sparrc · 2016-10-24T13:25:12Z

probably better to keep the PR as-is, IIRC empty tags get filtered out and ignored somewhere along the way anyways

phemmer · 2016-10-24T13:43:03Z

Yes, we can't merge this. Empty tags were deliberately removed because influxdb does not accept them:

# influx -database test
> insert test,foo= value=1
ERR: {"error":"unable to parse 'test,foo= value=1': missing tag value"}

sparrc · 2016-10-24T13:53:43Z

@phemmer telegraf isn't directly writing line-protocol from the tag/field maps provided to the accumulator, I'm fairly certain empty tags simply get dropped.

phemmer · 2016-10-24T13:56:02Z

I thought I remember them not getting dropped. But in any case, you're saying the net result is the same, empty tags don't get sent to influxdb. So why a code change that results in no effective behavior change?

sparrc · 2016-10-24T13:59:01Z

I believe this change allows the metric to get written, it will just get written without the empty tag. Whereas before we were dropping the metric altogether.

hahnjo · 2016-10-24T15:12:38Z

Right, that was the intention. I will do some tests what actually happens...

hahnjo · 2016-10-24T19:38:37Z

So, the current version of this works as intended, the tag will then be null in the database. However, I think it would indeed be cleaner to drop the empty tag. I will try this in the next few days...

The changes in #1848 resulted in lines being dropped if they had an empty tag. So let's count empty tags as valid to take the right decision!

hahnjo · 2016-10-25T20:02:51Z

This should be cleaner and also moves the empty check to a single place...

hahnjo · 2016-12-11T09:50:10Z

ping?

sparrc · 2016-12-13T14:21:51Z

@phemmer can you review this change as well?

phemmer · 2016-12-13T19:26:07Z

plugins/inputs/snmp/snmp.go

-	for _, r := range rows {
-		if len(r.Tags) < tagCount {
+	for i, r := range rows {
+		if len(r.Tags)+emptyTags[i] < tagCount {
 			// don't add rows which are missing tags, as without tags you can't filter
 			continue
 		}


Effectively this change is saying that adding measurements that have no tags is OK. If were going to make that change, this check (and other related code above) should just go away. Meaning if we allow tagless measurements into influxdb, we should allow them no matter the cause, whether it was because snmp was configured for a tag that ended up being empty and thus omitted, or whether it just wasn't configured with any tags at all.

Tagless measurements are already allowed into InfluxDB. I'm not sure I understand why the SNMP plugin would be asserting otherwise.

But if I understand correctly, @phemmer you're just saying that in that case this change is overly complicated? we can just remove any tag-checking at all rather than counting and incrementing this "emptyTags" map?

More or less. The emptyTags thing basically allows inserting measurements where the plugin was configured with a tag and the tag ended up being empty, but blocks measurements where there was no tag configured at all.
If we just allow tagless measurements period, then the emptyTags stuff goes away yes.

Okay, so I've implemented this and I would doubt that we want it that way. Anyway, I don't have a very strong opinion as long as I get my measurements back :-)

https://github.com/hahnjo/telegraf/commit/9699fcac21706619ddd33124626f01f17fd43ce0 (look out for the test changes and especially rtr4 which is added although the tag doesn't even exist in SNMP)

Okay, so I've implemented this and I would doubt that we want it that way

What way? The new way being discussed?
I'd be interested in hearing your arguments.

I'm really confused. The only difference between rtr4 and rtr3 is the missing myfield2 which isn't even a tag. If you are proposing that we drop based on a missing field, instead of dropping based on a missing tag, then both rtr3 and rtr4 would be dropped as they're both missing myfield4

No, the main difference from my perspective is that the defined tag is either empty or does not exist:

https://github.com/hahnjo/telegraf/commit/9699fcac21706619ddd33124626f01f17fd43ce0#diff-a240fd60347435c7253f4238fae424efL61 defines the test data. .1.0.0.0.1.1.2 is just empty for rtr3 while .1.0.0.0.1.1.3 doesn't exist for rtr4. I'd consider the latter an error which should be filtered out...

Anyway, should I open a new pull request with the new branch if that's the way you prefer?

There's no need to create a new PR. You can force-push the changes onto this branch and that will update this PR.

I know, but that's now a somewhat different approach to fix this

Ah, I think I see what you're after now. I'm on the fence about whether we should treat missing tag differently from empty tag. In the end, both result in a data point getting inserted with no tag, and the original intent of the dropping was to prevent data from getting inserted and lumped into a single series (no ability to filter & separate them).
@sparrc What behavior would you prefer?

Drop data point if a tag is missing or empty (current behavior)

Drop data point if a tag is missing, but allow if empty, even though when it gets to influxdb, the tag will still be missing because empty tags are removed (this PR)

Allow data points with both missing and empty tags (PR snmp: Allow lines with empty or missing tags #2172)

While I'm on the fence, I think I lean more towards # 3. I think I feel that if we want the behavior of # 2, it should be a generic telegraf option, similar to the tagpass/tagdrop options.

need to re-review

sparrc

see discussion above

hahnjo · 2016-12-16T19:05:43Z

Superseded by #2172

hahnjo changed the title ~~snmp: Don't ignore empty tags~~ snmp: Don't drop lines with an empty tag Oct 25, 2016

snmp: Don't drop lines with an empty tag

475e9d6

The changes in #1848 resulted in lines being dropped if they had an empty tag. So let's count empty tags as valid to take the right decision!

sparrc added this to the 1.2.0 milestone Oct 25, 2016

sparrc previously approved these changes Dec 13, 2016

View reviewed changes

phemmer suggested changes Dec 13, 2016

View reviewed changes

sparrc suggested changes Dec 16, 2016

View reviewed changes

hahnjo mentioned this pull request Dec 16, 2016

snmp: Allow lines with empty or missing tags #2172

Merged

2 tasks

hahnjo closed this Dec 16, 2016

phemmer mentioned this pull request Feb 11, 2017

Empty tag value causes error on InfluxDB output #2390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snmp: Don't drop lines with an empty tag #1927

snmp: Don't drop lines with an empty tag #1927

hahnjo commented Oct 23, 2016 •

edited

Loading

sparrc commented Oct 24, 2016

hahnjo commented Oct 24, 2016

sparrc commented Oct 24, 2016

phemmer commented Oct 24, 2016

sparrc commented Oct 24, 2016

phemmer commented Oct 24, 2016

sparrc commented Oct 24, 2016

hahnjo commented Oct 24, 2016

hahnjo commented Oct 24, 2016

hahnjo commented Oct 25, 2016

hahnjo commented Dec 11, 2016

sparrc commented Dec 13, 2016

phemmer Dec 13, 2016

sparrc Dec 13, 2016

phemmer Dec 13, 2016

hahnjo Dec 15, 2016

phemmer Dec 15, 2016

phemmer Dec 15, 2016

hahnjo Dec 16, 2016

sparrc Dec 16, 2016

hahnjo Dec 16, 2016

phemmer Dec 17, 2016

sparrc left a comment

hahnjo commented Dec 16, 2016

snmp: Don't drop lines with an empty tag #1927

snmp: Don't drop lines with an empty tag #1927

Conversation

hahnjo commented Oct 23, 2016 • edited Loading

Required for all PRs:

sparrc commented Oct 24, 2016

hahnjo commented Oct 24, 2016

sparrc commented Oct 24, 2016

phemmer commented Oct 24, 2016

sparrc commented Oct 24, 2016

phemmer commented Oct 24, 2016

sparrc commented Oct 24, 2016

hahnjo commented Oct 24, 2016

hahnjo commented Oct 24, 2016

hahnjo commented Oct 25, 2016

hahnjo commented Dec 11, 2016

sparrc commented Dec 13, 2016

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

sparrc left a comment

Choose a reason for hiding this comment

hahnjo commented Dec 16, 2016

hahnjo commented Oct 23, 2016 •

edited

Loading