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

adding the tags in the graylog output plugin #1861

Closed
wants to merge 3 commits into from

Conversation

ediezh
Copy link

@ediezh ediezh commented Oct 7, 2016

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

This is just to include the metric tags as well in the graylog output plugin, at the moment only metric values are included.

@sparrc
Copy link
Contributor

sparrc commented Oct 7, 2016

@vanillahsu can you review this please?

@edieship I'm not very familiar with graylog, but are there users who might not want tags in their graylog metrics? What sort of query capability does this add?

Copy link
Contributor

@vanillahsu vanillahsu left a comment

Choose a reason for hiding this comment

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

it seems ok to me.

@sparrc
Copy link
Contributor

sparrc commented Oct 7, 2016

are there any drawbacks to adding the tags?

@jamescarr can you review as well?

@ediezh
Copy link
Author

ediezh commented Oct 7, 2016

The reason I want to add the tags is because almost all the metrics come with different tags to better differentiate them. For example, for the docker container metrics, the container name is one tag, without the tags in the metric output, it'a very difficult to tell which container are the metrics for.

@sparrc
Copy link
Contributor

sparrc commented Oct 7, 2016

@edieship, it makes sense, TBH I'm surprised that the metrics were useful at all without the tags. I just want to make sure that current users of the plugin won't get negatively affected by this.

@jamescarr
Copy link
Contributor

Yeah this is useful because I also hit this problem. The tags are useful for filtering in graylog (for example, I commonly want to look at logs for a particular zone).

@sparrc sparrc added this to the 1.1.0 milestone Oct 7, 2016
@sparrc
Copy link
Contributor

sparrc commented Oct 7, 2016

OK, @edieship you can update the changelog and I'll merge

@sparrc
Copy link
Contributor

sparrc commented Oct 7, 2016

cherry-picked the commits and made some tweaks to avoid some unnecessary allocations in the hash map: #1865

@sparrc sparrc closed this in #1865 Oct 7, 2016
jackzampolin pushed a commit that referenced this pull request Oct 7, 2016
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.

4 participants