Skip to content

snmp: return error on unknown conversion type#1853

Merged
sparrc merged 1 commit intoinfluxdata:masterfrom
phemmer:snmp-error-unknown-conversion
Oct 6, 2016
Merged

snmp: return error on unknown conversion type#1853
sparrc merged 1 commit intoinfluxdata:masterfrom
phemmer:snmp-error-unknown-conversion

Conversation

@phemmer
Copy link
Contributor

@phemmer phemmer commented Oct 5, 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)

Since the influxdb models marshaller panics on byte slices (when not a valid line-protocol value), we should throw an error instead of passing through the value unchanged.

I didn't update the changelog or anything as this isn't really a bug fix or enhancement. Unless people have misconfigured telegraf, there should be no behavior change.

closes #1852

@phemmer
Copy link
Contributor Author

phemmer commented Oct 5, 2016

I'm also wondering if we should capture panics coming from m.pt.Fields(). Just as protection from any other plugins which might send data that can't be marshalled.

I find it rather odd that a panic is thrown instead of returning an error. I know the current signature of the call doesn't return error, but maybe it should.

@sparrc
Copy link
Contributor

sparrc commented Oct 6, 2016

Doesn't seem like it should be panicking, but we are using a fairly old changeset of the influxdb repo, and the latest might have a different behavior.

I'll be updating the dependency in #1777

@phemmer
Copy link
Contributor Author

phemmer commented Oct 6, 2016

Doesn't look like the current code is any different: https://github.com/influxdata/influxdb/blob/master/models/points.go#L1769

@sparrc sparrc merged commit 5a86a2f into influxdata:master Oct 6, 2016
@phemmer phemmer deleted the snmp-error-unknown-conversion branch October 6, 2016 13:24
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.

Panic in Telegraf with new snmp plugin using conversion in 1.0.1

2 participants