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

Initial implementation for multiple outputs #59

Merged
merged 7 commits into from
Aug 11, 2015

Conversation

jipperinbham
Copy link
Contributor

This is sort of a phase one to create an outputs configuration in order to send metrics to more than InfluxDB and begins to address #38.

I'd love some feedback here!

_ "github.com/influxdb/telegraf/plugins/all"
"github.com/jipperinbham/telegraf"
Copy link
Contributor

Choose a reason for hiding this comment

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

These will need to be fixed before merging this PR

@sparrc
Copy link
Contributor

sparrc commented Aug 5, 2015

@jipperinbham Hi JP, if you want to fix the 2 comments I made and the branch conficts, then I can test this out and merge it!

@jipperinbham
Copy link
Contributor Author

@sparrc I've updated the import paths to point to influxdb and rebased from master. Please let me know if something isn't building or running properly.

@sparrc
Copy link
Contributor

sparrc commented Aug 6, 2015

@jipperinbham It looks like there may have been a merge problem, see ci failure above:

./config.go:220: undefined: ErrInvalidConfig Action failed: go build

@jipperinbham
Copy link
Contributor Author

sorry about that @sparrc, should be good to test now

@sparrc
Copy link
Contributor

sparrc commented Aug 6, 2015

@jipperinbham no prob!

This looks good to me, I like the design and fits with telegraf well.

Two questions, and maybe I'm just not 100% understanding the code, but should the sample and test TOML files be changed from [influxdb] to [outputs] ?

2nd: is there any validation that a valid output was specified? not that it needs to validate that the output destination is connectable, but maybe some validation that the configured output exists as an "output plugin" in telegraf?

PS: you can find those files in etc/config.sample.toml and testdata/influx.toml

@jipperinbham
Copy link
Contributor Author

re: the sample TOML files, I held off on updating those in case there were some changes I needed to make but will do that now

re: output validation, I'm pretty sure the following covers it

https://github.com/jipperinbham/telegraf/blob/outputs-phase1/agent.go#L83-L86

@sparrc
Copy link
Contributor

sparrc commented Aug 7, 2015

@jipperinbham I think you've accidentally added the telegraf binary to your commit, once that's removed I think the PR is good to go 👍

@sparrc
Copy link
Contributor

sparrc commented Aug 10, 2015

@jipperinbham I'm anxious to get this merged, are you done with this PR? I think last we left it, you needed to update the Connect() function?

@jipperinbham
Copy link
Contributor Author

@sparrc yes, it should be good to go. Sorry I forgot to /cc you on that last commit.

I couldn't fully test because I hit the line protocol change for deprecating ints but all the config and tags looked good.

@sparrc sparrc merged commit 91f6c4b into influxdata:master Aug 11, 2015
sparrc added a commit that referenced this pull request Aug 11, 2015
sparrc added a commit that referenced this pull request Aug 11, 2015
This reverts commit 48a0755, reversing
changes made to 924700f.
@sparrc
Copy link
Contributor

sparrc commented Aug 11, 2015

@jipperinbham I tried to merge this in but had some major conflicts, so I had to revert it, unfortunately I don't think that I'm able to re-open this PR.

Sorry for the hassle, but could you please rebase your changes and re-open a new PR?

@jipperinbham
Copy link
Contributor Author

@sparrc no problem, I'll rebase and get things cleaned up and submit a new PR

@sparrc
Copy link
Contributor

sparrc commented Aug 11, 2015

thanks @jipperinbham! Just to warn you, that is probably going to be a pretty hairy merge/rebase, there have been some pretty large changes in agent.go and config.go.

Let me know if you need a hand and we can walk through it together on Google hangouts if need be

@jipperinbham
Copy link
Contributor Author

It looks like the issue is here, 29e8ce6, which I'm not entirely sure how to handle since we moved decided to move the Tags to the InfluxDB output and out of Config.

Not entirely sure how to proceed with the changes needed...

@sparrc
Copy link
Contributor

sparrc commented Aug 11, 2015

@jipperinbham yes, this is what I was afraid of, I think I may have been wrong to suggest moving that into InfluxDB, and that the Tags should in fact stay in Config.

@jipperinbham
Copy link
Contributor Author

no worries, I can easily look back through the git commits to move it back, it also makes cleaning up these merge conflicts possible without touching what others have added.

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