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

Add snmp input plugin #546

Closed
wants to merge 1 commit into from
Closed

Conversation

titilambert
Copy link
Contributor

#495 rebased with master

@titilambert titilambert changed the title Add snmp plugin Add snmp input plugin Jan 19, 2016
# snmptranslate -m all -Tz -On | sed -e 's/"//g' > /tmp/oids.txt
# Or if you have an other MIB folder with custom MIBs
# snmptranslate -M /mycustommibfolder -Tz -On -m all | sed -e 's/"//g' > oids.txt
snmptranslatefile = "/tmp/oids.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

change to snmptranslate_file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem !

@sparrc
Copy link
Contributor

sparrc commented Jan 19, 2016

@titilambert I still don't agree with the snmptranslate file implementation. It seems that users can specify a file, but then they still need to define each OID in the config anyways?

Why not just use all OIDs from the file AND OIDs defined in the config? Why do they need to be defined both places?

// SNMP getbulk max repetition
MaxRepetition uint8 `toml:"max_repetition"`
// We want to resolve oid to names oids text file
Snmptranslate bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This option seems unnecessary, why define the OID in two places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question ! In fact this option define if you want to use the name from Name option in the section or if you want to use the name from snmptranslate_file.
So, maybe we can delete this option and do:

  1. Get name from snmptranslate_file if it exists
  2. Get name from Name option in the section
    In that case we can not use OID as name (.1.3.6.....) which is good I think

Are you agree with that ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that sounds good 👍

@titilambert
Copy link
Contributor Author

Hello !

@titilambert I still don't agree with the snmptranslate file implementation. It seems that users can specify a file, but then they still need to define each OID in the config anyways?

Why not just use all OIDs from the file AND OIDs defined in the config? Why do they need to be defined both places?

@sparrc I'm partially agree with you :)
The user should use both oids and names in telegraf conf files:

  • Names: because, as you said, you don't want to rewrite oids
  • OID: because when you use bulk requests, you just want the result oids translated, so the main oid has no important and could not have names ...

So, I will add this option to use names and oids if you agree with that.

Thanks !

@sparrc
Copy link
Contributor

sparrc commented Jan 19, 2016

@titilambert I think I understand, sounds good to me 👍

@titilambert
Copy link
Contributor Author

@sparrc Perfect I will also update the sample-config to get more examples...

titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
titilambert added a commit to titilambert/telegraf that referenced this pull request Jan 20, 2016
@titilambert titilambert force-pushed the snmp branch 3 times, most recently from 9d997b3 to eee5feb Compare January 20, 2016 17:14
@titilambert
Copy link
Contributor Author

@sparrc Hello ! I think this one is good ! :)

@titilambert
Copy link
Contributor Author

@sparrc and rebased again :D :D

@sparrc
Copy link
Contributor

sparrc commented Jan 20, 2016

Thanks @titilambert, now that I've been playing around with it I'm starting to understand better. This is a very powerful and configurable plugin, but I do find it rather complicated to use.

I was wondering what you thought of making the configuration look more like this:

[[inputs.snmp]]
  snmptranslate_file = "/tmp/oids.txt"
  address = "192.168.2.2:161"
  # SNMP community
  community = "public" # default public
  # SNMP version (1, 2 or 3)
  # Version 3 not supported yet
  version = 2 # default 2
  # SNMP response timeout
  timeout = 2.0 # default 2.0
  # SNMP request retries
  retries = 2 # default 2
  # Which get/bulk do you want to collect for this host:
  get_oids = [
    "ifNumber",
    "snmpInPkts",
    "snmpOutPkts",
    ".1.3.6.1.2.1.1.3.0",
  ]
  bulk_max_repetition = 127
  bulk_oids = [
    ".1.3.6.1.2.1.1",
  ]

This is a lot more simple, but obviously this also loses some of the config granularity. If this over-simplifies the necessary features of SNMP you can just let me know, I'm certainly not an snmp expert.

}
}
// Deconnection
defer snmpClient.Conn.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

This defer should be moved up to line 264, directly after the client is made (but after the error is checked)

@sparrc
Copy link
Contributor

sparrc commented Jan 20, 2016

also -- could you make the testdata/oids.txt file smaller?

@titilambert
Copy link
Contributor Author

@sparrc

also -- could you make the testdata/oids.txt file smaller?

Done !

@titilambert
Copy link
Contributor Author

@sparrc Unfortunately, bulk request need at least this granularity... :/ In fact, I thought to add more options (in a future version) to get a better support of SNMP tables (#540).
The issue with SNMP is this protocol is toooo old, so we need to make a lot of stuff ourself to get something flexible :/
But I have to admit, the config will be not really easy ...

BTW, do we have something for user documentation ? (like sphinx/readthedocs in python)

@sparrc
Copy link
Contributor

sparrc commented Jan 21, 2016

@titilambert I had a feeling it would be the case :-)

One final request, I think that there could be an "easy" config for someone looking to get started quickly with some SNMP metrics. I was thinking that you could add an option to the snmp "host" where users can just specify a simple list of "get" OIDs.

I realize this wouldn't work for bulk or table requests, but I think it could still be useful. At least from my perspective I would like to be able to just monitor a few local SNMP metrics on my workstation.

[[inputs.snmp.host]]
  address = "localhost"
  # SNMP community
  community = "public" # default public
  # SNMP version (1, 2 or 3)
  # Version 3 not supported yet
  version = 2 # default 2
  # SNMP response timeout
  timeout = 2.0 # default 2.0
  # SNMP request retries
  retries = 2 # default 2

  collect = [...]

  # Simple list of OIDs to get, in addition to "collect"
  get_oids = [
    "ifName",
    "snmpInPkts",
    "snmpOutPkts",
    ".1.3.6.1.2.1.1.3.0",
  ]

@titilambert
Copy link
Contributor Author

@sparrc changes done !
I will test it real environnement next week. Do you prefer to wait before merging ?
Thanks !

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

Successfully merging this pull request may close these issues.

3 participants