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

ES output: allow to use tag values on index name #3470

Merged
merged 8 commits into from
Nov 21, 2017
Merged

ES output: allow to use tag values on index name #3470

merged 8 commits into from
Nov 21, 2017

Conversation

lpic10
Copy link
Contributor

@lpic10 lpic10 commented Nov 14, 2017

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Should implement #3442

This should have some effect on performance but I haven't measured. I'm not go expert, there may be better ways of doing the same.

Let me know if this is ok, so I update the doc as well.

@danielnelson
Copy link
Contributor

If a user has an environment variable set with the same name as the tag you might have problems, since we use the same ${var} syntax for these. We could use the Go template package https://golang.org/pkg/text/template/, it would look like foo-{{.host}}, they are a little bit overkill though in terms of what you could do with them.

@lpic10
Copy link
Contributor Author

lpic10 commented Nov 14, 2017

If a user has an environment variable set with the same name as the tag you might have problems, since we use the same ${var} syntax for these.

Ops, yeah, completely forgot about that... I will have a look at the template package.

@lpic10
Copy link
Contributor Author

lpic10 commented Nov 15, 2017

Well, at the end it was much simpler to just change the notation to foo-{{host}} than using template.

} else {

tagName := indexName[startTag+2 : endTag]
tagNameTrim := strings.TrimSpace(tagName)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we trim the space here, then I think the tag may not match further down in the replacer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I kept the the tagName variable to be used by the replacer instead of trimming on it directly

tagNameTrim := strings.TrimSpace(tagName)
found := false

for k := range metricTags {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this happens for each metric, I'm concerned about the performance. Perhaps this can be improved with by doing some of the work at Connect time.

I'll try to explain in more detail, this part needs only done once:

  • Make an array of tagkeys: ["host"]
  • Build a format string from the indexName template with the tags replaced with %s: telegraf-%s-%Y.%m.%d

Then at Write time:

  • Lookup the tag values and place them into an array.
  • Use fmt.Sprintf with the format string and the array of tag values
  • Use the existing replacer for the dates.

@lpic10
Copy link
Contributor Author

lpic10 commented Nov 16, 2017

@danielnelson I tried to implement the changes you requested, let me know.

@danielnelson
Copy link
Contributor

Code looks perfect, can you rebase on master to fix the file conflict?

@danielnelson danielnelson merged commit 92ca661 into influxdata:master Nov 21, 2017
@danielnelson danielnelson added this to the 1.5.0 milestone Nov 21, 2017
@danielnelson danielnelson added area/elasticsearch feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 21, 2017
@lpic10 lpic10 deleted the issue3442 branch November 23, 2017 09:48
aromeyer pushed a commit to aromeyer/telegraf that referenced this pull request May 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/elasticsearch feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants