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 table feature #540 #735

Closed
wants to merge 3 commits into from
Closed

Conversation

titilambert
Copy link
Contributor

@sparrc Hello !
It seems tests are failing because of number of entries in config files ???

telegraf -sample-config > testconfig.conf
telegraf -config  testconfig.conf

panic: runtime error: slice bounds out of range [recovered]
    panic: runtime error: slice bounds out of range

goroutine 1 [running]:
github.com/naoina/toml.(*parseState).execute.func1(0xc8200b17f8)
    /home/tcohen/gits/go/src/github.com/naoina/toml/parse.go:47 +0xab
github.com/naoina/toml.(*tomlParser).Execute(0xc8200bf880)
    /home/tcohen/gits/go/src/github.com/naoina/toml/parse.peg.go:849 +0x3ca
github.com/naoina/toml.(*parseState).execute(0xc8200b18c0, 0x0, 0x0)
    /home/tcohen/gits/go/src/github.com/naoina/toml/parse.go:52 +0x63
github.com/naoina/toml.(*parseState).parse(0xc8200b18c0, 0x0, 0x0)
    /home/tcohen/gits/go/src/github.com/naoina/toml/parse.go:38 +0x1e4
github.com/naoina/toml.Parse(0xc8201b6000, 0x84dc, 0x86dc, 0x84dc, 0x0, 0x0)
    /home/tcohen/gits/go/src/github.com/naoina/toml/parse.go:15 +0xfd
github.com/influxdata/config.ParseFile(0x7ffc199b6417, 0xf, 0xc8200b1978, 0x0, 0x0)
    /home/tcohen/gits/go/src/github.com/influxdata/config/legacy.go:50 +0xaa
github.com/influxdata/telegraf/internal/config.(*Config).LoadConfig(0xc8204092d0, 0x7ffc199b6417, 0xf, 0x0, 0x0)
    /home/tcohen/gits/go/src/github.com/influxdata/telegraf/internal/config/config.go:328 +0x61
main.main()
    /home/tcohen/gits/go/src/github.com/influxdata/telegraf/cmd/telegraf/telegraf.go:147 +0xd2e

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
    /usr/lib/go/src/runtime/asm_amd64.s:1721 +0x1

goroutine 5 [syscall]:
os/signal.loop()
    /usr/lib/go/src/os/signal/signal_unix.go:22 +0x18
created by os/signal.init.1
    /usr/lib/go/src/os/signal/signal_unix.go:28 +0x37

goroutine 34 [chan send]:
github.com/naoina/toml.(*tokens16).Tokens.func1(0xc8200d3110, 0xc820080580)
    /home/tcohen/gits/go/src/github.com/naoina/toml/parse.peg.go:466 +0x134
created by github.com/naoina/toml.(*tokens16).Tokens

If I remove 4 entries in testconfig.conf file, I don't have any error ...

Is it a bug from toml ???

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

yes, I ran into this recently too, can you paste in your entire config file and I can help? It seems like the toml parser doesn't like ### or the large Telegraf Config section at the top

@titilambert
Copy link
Contributor Author

here the config file: https://gist.github.com/titilambert/e5572dd1223a85c5d2af
Should we patch toml lib ?

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

try replacing all of the ## with just #

@titilambert
Copy link
Contributor Author

@sparrc It's working ! We should catch the error and print a better error message for users

@titilambert
Copy link
Contributor Author

@sparrc It's NOT workgin (I load an other config file :/)

@titilambert
Copy link
Contributor Author

@sparrc I just updated the gist

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

what about:

#
#                                  OUTPUTS                                    #
#

to just

# OUTPUTS

@titilambert
Copy link
Contributor Author

Still no :( (gist updated)

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

@titilambert
Copy link
Contributor Author

Still no :(
But I remove the 29 last lines (on both files):

The only diff is : in the working file I just commented the last line ....
I don't get why !??

@titilambert
Copy link
Contributor Author

@sparrc are we stuck with this parser ?
It seems dead since 08/2015... I saw that all influxdata softs are using it but maybe you should consider to change the dependency to an other parser like https://github.com/pelletier/go-toml or https://github.com/kezhuw/toml

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

I tried pelletier/go-toml but it doesn't provide the capability to unmarshal toml data onto an arbitrary interface. https://github.com/kezhuw/toml looks promising.

@titilambert
Copy link
Contributor Author

@sparrc So what can I do ? :D

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

I really don't know, sorry, just try to get the file to not panic 🙈 🔫

@titilambert
Copy link
Contributor Author

:D oki, I will try something this evening :)

@titilambert
Copy link
Contributor Author

@sparrc I made a "dirty workaround" which removes all comments from sampleconfig test file when you run circle-test.sh script
If it working I could make an other PR just for this ...

@titilambert
Copy link
Contributor Author

@sparrc well, it's working, it's ugly ... this is workaround definition ...
What do you think about ?

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

😱! no! this is catching an actual bug, the -sample-config generated file should work out of the box. Try rebasing and running again (I made some changes to the config file format when merging a separate PR just now)

@titilambert
Copy link
Contributor Author

@sparrc :D :D

@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

@titilambert I've merged all necessary external PRs and fixed dependencies for #745, please rebase and the config file panic should be fixed.

@titilambert
Copy link
Contributor Author

@sparrc rebased !
Thanks !!

@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

OK, is this ready to go then? Could you update the README with an explanation of what these table features are exactly?

@titilambert
Copy link
Contributor Author

@sparrc README added !
I think this is ready !

@sparrc
Copy link
Contributor

sparrc commented Mar 2, 2016

in the README, can you put more explanation of the different config options? I'm not sure I understand the differences between inputs.snmp.table and inputs.snmp.host.table? And what is a subtable? Are these at all related to snmptable?

@titilambert
Copy link
Contributor Author

@sparrc README updated ! Tell me if is OK now.
Please wait before merging, I just want to check something before (#TODELETE stuff in readme)

@titilambert
Copy link
Contributor Author

#TODELETE stuff removed :)

@sparrc This PR is good for me !

@sparrc sparrc closed this in 72027b5 Mar 3, 2016
@titilambert titilambert deleted the snmp branch March 7, 2016 19:58
@titilambert titilambert restored the snmp branch March 7, 2016 19:58
@titilambert titilambert deleted the snmp branch March 7, 2016 19:58
geodimm pushed a commit to miketonks/telegraf that referenced this pull request Mar 10, 2016
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.

2 participants