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

allow specifying field name in graphite template #4178

Merged
merged 10 commits into from
Oct 8, 2015
Merged

allow specifying field name in graphite template #4178

merged 10 commits into from
Oct 8, 2015

Conversation

roobert
Copy link
Contributor

@roobert roobert commented Sep 20, 2015

Initial attempt at implementing this: #4176

I'd like some feedback on how this should be implemented, primarily:

  • should multiple field tags be allowed, in the same way as the measurement tag is?
  • is something like this valid: mytag.measurement*.field
  • should field* be implemented

I also obviously need to add some tests but thought I'd try and get a discussion going on implementation first..

@otoolep
Copy link
Contributor

otoolep commented Sep 24, 2015

Thanks @roobert -- test failures can be seen here:

https://circleci.com/gh/influxdb/influxdb/6023

@roobert
Copy link
Contributor Author

roobert commented Sep 24, 2015

@otoolep - thanks, any comments on my original post?

@roobert
Copy link
Contributor Author

roobert commented Oct 7, 2015

@otoolep: I think this is ready to be merged, is there anything else I need to do?

I've added tests, signed the CLA, and updated the graphite listener README.md.

This patch doesn't change any existing functionality and doesn't modify the default behaviour of the graphite listener.

We've been running this in production for 4 days and all seems good so far with ~40,000 points a second.

@otoolep
Copy link
Contributor

otoolep commented Oct 8, 2015

cc @jwilder

@jwilder
Copy link
Contributor

jwilder commented Oct 8, 2015

Looks good @roobert! 👍

@roobert
Copy link
Contributor Author

roobert commented Oct 8, 2015

@otoolep @jwilder, cheers peeps, when can this be merged?

@roobert
Copy link
Contributor Author

roobert commented Oct 8, 2015

@sparrc - you seem to be working on the graphite parser, is this something you could look into merging?

Cheers,

@sparrc
Copy link
Contributor

sparrc commented Oct 8, 2015

@roobert, looks good, thank you, I can merge this

@otoolep
Copy link
Contributor

otoolep commented Oct 8, 2015

+1, thanks @roobert

@sparrc sparrc merged commit f3e3bf7 into influxdata:master Oct 8, 2015
sparrc added a commit that referenced this pull request Oct 8, 2015
@roobert
Copy link
Contributor Author

roobert commented Oct 9, 2015

👍 thanks for this!

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.

None yet

4 participants