Skip to content
This repository was archived by the owner on Feb 26, 2020. It is now read-only.

Automattic's Fork #3

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

Automattic's Fork #3

wants to merge 49 commits into from

Conversation

xyu
Copy link

@xyu xyu commented Aug 11, 2014

Howdy,

First of all sorry about this huge pull request, mainly I wanted to get a conversation going to see if you guys would be interested in merging some of the stuff from Automattic's fork of your plugin back in. We've made some extensive changes but mainly this fork:

  • Renames some stats to conform with ES native JSON API's naming schemes more
  • Is updated to work with multi node clusters without over-reporting index stats
  • Reports node stats for all nodes
  • Is ES 1.x compatible

Let me know if you guys are interested in merging in any of these features from our fork.

Danilo Penna Queiroz and others added 30 commits May 27, 2014 19:03
Because we are sending stats to StatsD on an interval everything is a gauge
even "counts" which are ever increasing. There's a current issue open for
StatsD (statsd/statsd#324) which requests "absolute" counters which would
solve this problem however until that's solved we have to use gauges and
just apply derivative functions after the fact. Le Sigh.
Add a config to turn on reporting of node level index stats (e.g. docs per
node) which defaults to false.
@ssimeonov
Copy link
Contributor

@xyu thanks for doing all this work and preparing a PR: this is awesome and we'd love to evaluate merging the changes. That said, this PR is difficult to review because it has too many commits which have different granularities and are about different logical changes.

Please, consider doing the following:

  1. Do an interactive rebase to organize commits into logical groups
  2. Squash minor commits to reduce the total number of commits. It helps to reduce the noise as people look at the git log. Make sure there are good commit messages that describe what's happening and why.
  3. Submit multiple PRs. Independent PRs can each be based off of master. PRs dependent on other PRs should either wait until the changes are merged or be stacked on top of the previous PRs.

Thanks!

@xyu
Copy link
Author

xyu commented Aug 11, 2014

Right of course :) but seeing as there are some rather major changes that break backwards compatibility with both StatsD key names and perhaps with ES 0.90.x (untested) I wanted to see if Swoop would even be interested in this fork.

Are there features you guys would like and other you would rather avoid and how concerned are you guys with backwards compatibility?

@ssimeonov
Copy link
Contributor

@xyu I created the https://github.com/swoop-inc/elasticsearch-statsd-plugin/tree/v1x branch for your PRs to target. Once we are done with the integration there we can switch master to the new version.

@dongshengbc
Copy link

@xyu awesome work

xyu added 3 commits August 20, 2014 11:57
The newest version (v3.0.2) of java-statsd-client now supports doubles for
guage value and gauge deltas, update ES StatsD Plugin to send doubles
instead of longs for load averages.

This resolves #1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants