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

fixes Bug 853299: added statsd to legacy processor #1147

Merged
merged 1 commit into from Mar 25, 2013
Merged

fixes Bug 853299: added statsd to legacy processor #1147

merged 1 commit into from Mar 25, 2013

Conversation

twobraids
Copy link
Contributor

This PR adds statsd support to the new processor. It was evidently not updated when statsd was added to the old processor. This oversight is corrected.

I added a statsd wrapper class that adds configman support to the StatsClient class. It offers a dynamic loading of statsd and the ability to turn counters on and off in configuration.

@lonnen
Copy link
Contributor

lonnen commented Mar 21, 2013

Re-ran this in leeroy because the failure seemed unrelated to the patch, but the same failure reoccured (see builds 600, 601). It is not affecting other PRs, or socorro-master.

@lonnen
Copy link
Contributor

lonnen commented Mar 22, 2013

@twobraids what was the problem earlier?

@twobraids
Copy link
Contributor Author

I wish I knew. I started a new branch based on a later master and introduced my changes one at a time. The sum of the parts passed. Then I rebased to one commit and that also passed. No clue as to what was the actual cause of the problem.

# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

empty doc string?

@lonnen
Copy link
Contributor

lonnen commented Mar 24, 2013

@twobraids ok. Happy to just move on from that.

I'm not sure about the empty docstring and did you consider metrics instead of statistics as the module name? Otherwise this is a-ok with me.

@lonnen
Copy link
Contributor

lonnen commented Mar 24, 2013

One more thought: single segments of a metric that may contain dots, like hostnames, should have the dots replaced with _. This can be done by sanitizing risky keys, or by changing the stats module to accept lists of strings, sanitizing the list, and '.'join(that_list). I'm thinking the later is overkill.

Come to think of it, nothing jumps out at me here as a risky key... maybe the Processor name? Ignore this bit if I've described a problem we aren't having.

@twobraids
Copy link
Contributor Author

I think the method currently in use is a faithful reproduction of the method currently in use in the production processor.

lonnen added a commit that referenced this pull request Mar 25, 2013
fixes Bug 853299: added statsd to legacy processor
@lonnen lonnen merged commit 808a340 into mozilla-services:master Mar 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants