-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
GREEDY field templates for the graphite parser #789
GREEDY field templates for the graphite parser #789
Conversation
@@ -94,6 +94,13 @@ func TestTemplateApply(t *testing.T) { | |||
measurement: "cpu.load", | |||
tags: map[string]string{"zone": "us-west"}, | |||
}, | |||
{ | |||
test: "conjoined fields", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you should test that the correct fields get applied.
ie, this should test that measurement=="cpu.util" and field=="idle.percent" (feel free to make a separate test function for testing field templates)
@chrusty can this also support specifying multiple fields?, ie, |
@sparrc OK, I've added a test for the field-name. Regarding the multiple-fields question, no at the moment it doesn't, but I could add it. I found this example in another issue (#39):
I'm skiing this week, but i'll see if i get a bad-weather day to take a look. |
@chrusty any updates? |
@sparrc So the initial part of the PR seems to work fine, but I'm struggling with the "multiple fields" thing... is there an accepted way of sending multi-value metrics (multi-field) with StatsD, or would people just send several metrics with different field names instead? It looks like you can send several metrics for a measurement, but I believe that StatsD would treat those as different metrics for the same measurement (not different fields for the same metric). |
@chrusty that's not what I had in mind exactly, I was thinking a template like this:
would translate graphite to this:
|
_, _, _, err = p.ApplyTemplate("current.users.logged_in") | ||
if err == nil { | ||
t.Errorf("Parser.ApplyTemplate unexpected result. got %s, exp %s", err, | ||
"'field' can only be used once in each template: current.users.logged_in") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove this constraint to allow the new feature.
@sparrc I think this does what you want. Sorry, I'd assumed that the feature you'd asked for was much more complex! |
…ple specific field names
No description provided.