-
Notifications
You must be signed in to change notification settings - Fork 4
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
90%: Tripod stats factory #94
Conversation
Conflicts: test/unit/mongo/MongoTripodDriverTest.php
Some questions that are probably best answered by @RobotRobot, which would be relevant to either this PR or #70:
|
* @var array The original read preference gets stored here | ||
* when changing for a write. | ||
*/ | ||
private $originalCollectionReadPreference = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have these been removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's from #70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch was branched off of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually see any indication that they were referenced anywhere. It looks like it might have been intended to be what the same properties on Updates.class.php became.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok
Added a lot of new stats calls: I noticed that we really only called I also added a 'custom' method to ITripodStat which allows the dev to keep arbitrary stats besides counts and timers. StatsD has 'gauges', so for that class, this custom method just uses that. With this, I've added stats for how many subjects are in an ApplyOperation job and how many impacted subjects are found in a DiscoverImpactedSubjects job. I don't really know at what threshold these stats become noise (or problematic to graphite), but I figure I can add a lot in the PR (where we can see them all) and rip them out after discussion if need be. |
Ah, I noticed that for StatsD->timer(), we're also sending an increment ( |
define('MONGO_CREATE_SEARCH_DOC','MONGO_CREATE_SEARCH_DOC'); | ||
define('MONGO_CONNECTION_ERROR','MONGO_CONNECTION_ERROR'); | ||
|
||
define('STAT_TYPE_COUNT', 'count'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this to SUBJECT_COUNT, that's what you're counting.
As discussed I think the use of gauges is wrong. Gauges measure a value in a single context, i.e. the speed of a single car, the %load of a CPU, at a single point in time. So what you have here is analogous to the amount of subjects being processed at any one time. The last job to post a value will overwrite any other jobs running in parallel in a given time window. Instead I think we should simplify this to be able to increment by n (default 1). That way each parallel running job just contributes to the value by incrementing it in a given minute, not overwriting the value of others. |
New stats added by the branch:
|
This builds on the work from #70 but takes a somewhat different approach to how Stats objects are passed around.
This maintains the current Tripod pattern of passing a Stats object to Tripod\Mongo\Driver as part of the constructor, then passes enough stats config for the background jobs to instantiate the same stats objects from a factory class.