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

#4699 Improve how the datadog plugin uses Metric.Fields and Metric.Tags #4803

Merged
merged 2 commits into from
Oct 5, 2018

Conversation

kevinconaway
Copy link
Contributor

Required for all PRs:

  • [Y ] Signed CLA.
  • [ NA] Associated README.md updated.
  • [ NA] Has appropriate unit tests.

@danielnelson This PR addresses the feedback in #4699. I replaced the use of Metric.Fields with Metric.FieldList and reduced how often Metric.Tags is called (it didn't look like using TagList made sense)

Since the changes were internal, I left the existing tests as is.

@@ -76,6 +76,10 @@ func (d *Datadog) Write(metrics []telegraf.Metric) error {

for _, m := range metrics {
if dogMs, err := buildMetrics(m); err == nil {
tagMap := m.Tags()
metricTags := buildTags(tagMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much better but if you want to take it even further you could use TagList() to build the tags list. TagList is always sorted so you can avoid the sort as well.

@danielnelson danielnelson added the performance problems with decreased performance or enhancements that improve performance label Oct 3, 2018
@danielnelson danielnelson added this to the 1.9.0 milestone Oct 3, 2018
@danielnelson
Copy link
Contributor

Tests should pass if you add the changes made by make fmt.

@kevinconaway
Copy link
Contributor Author

@danielnelson Thanks for the feedback!

I made the changes that you suggested for buildTags to use Metric.TagsList(). I also removed the call to Metric.Tags() in favor of just looking up the host tag directly on the metric. There was no need to a new map just to find that one tag.

@danielnelson danielnelson merged commit 422c142 into influxdata:master Oct 5, 2018
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance problems with decreased performance or enhancements that improve performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants