[Asterisk-code-review] StatsD: Add sample rate compatibility (asterisk[master])
Ashley Sanders
asteriskteam at digium.com
Tue Nov 10 15:09:18 CST 2015
Ashley Sanders has posted comments on this change.
Change subject: StatsD: Add sample rate compatibility
......................................................................
Patch Set 1: Code-Review-1
(13 comments)
https://gerrit.asterisk.org/#/c/1571/1//COMMIT_MSG
Commit Message:
Line 9: Added code that allowed for the user to declare the value of the
: sample rate. If the user does not declare a sample rate, then a
: rate of 1.0 is sent by default.
I would rephrase this to say something like, "Implemented support for StatsD sampleRate parameter, which is a parameter for determining when to send computed statistics to a client.
Valid values for the parameter are:
- <1.0 (statistics will be sent to the client at random)
- =1.0 (statistics are always sent to the client)
https://gerrit.asterisk.org/#/c/1571/1/apps/app_statsd.c
File apps/app_statsd.c:
Before I go any further with the code review, I am going to suggest that you do a little bit of refactoring (which will make some of my review comments moot, but that's okay.)
What I see is the following:
1. The application is executed...
2. Arguments are validated.
3. You then validate the metric type, the statistic name, and the value, each independently.
4. Because each of the above validation routines are generic for 'all' types, your functions are sort of weird in that they have to over and over see what kind of metric we are validating and contain a lot of built-in logic to say 'if we are dealing with type n, then apply this business logic. else, apply this other logic.' You can eliminate a lot of that by partitioning the code a bit differently.
I think a better approach would be to figure out what type of metric you are validating and use the strategy pattern to have a function to validate each argument set based on the type of metric. In other words, validate_metric_type_set, validate_metric_type_gauge, etc.
Line 45: Valid metric types
: are 'g' for gauge, 'c' for counter, 'ms' for timer, and 's' for
: sets.
Do we have the ability to denote things in a list form within a paragraph? I don't know the answer off-hand, but if we do, this is definitely a place to use that ability.
Line 54: Values
: must be numeric. Values for gauge and counter metrics can be
: sent with a '+' or '-' to update a value after the value has
: been initialized. Only counters can be initialized as negative.
: Sets can send a string as the value parameter, but the string
: cannot contain the pipe character.
We need to break this paragraph up somewhat. When writing technical documentation, it's best to keep your statements terse and separate distinct thoughts into their own paragraphs or list elements (e.g. maybe have a separate paragraphs, where each paragraph only discusses the mechanics for one type of metric.)
Line 64: greater than or equal to 1 will always be sent, and any rate
: between 1 and 0 will be left up to chance.</para>
> Inside of res_statsd.c, where this sends data to, if the sample rate is bet
I think you should consider using something more akin to what I used in my review comment for the commit message:
Valid values for the sample_rate are:
<1.0 (statistics will be sent to the client at random)
=1.0 (statistics are always sent to the client)
Line 83: This function checks to see if the value given to the StatsD daialplan
: * application is within the allowed range as specified by StatsD. A counter
: * is the only metric type allowed to be initialized as a negative number.
You should state what the allowed range is in the description, which seems to be -2^63 to 2^63.
Line 120: validate_metric
This should be validate_metric_type.
Line 130: for (i = 0; i < ARRAY_LEN(valid_metrics); i++) {
: if (!strcmp(valid_metrics[i], metric)) {
: return 0;
: }
: }
I don't understand why this logic is sufficient. Consider the following, if a user gives you a string like:
groovygeeks
coolcat
blackberryjamsgreatstuff
madsoftwarescientist
This function will evaluate each of those as valid because each contains one (or more) of the legal characters for metrics. (Since your function exits upon the first legal character or series of characters it detects, the remaining legal character(s) would be ignored.)
Is this how statsd validates input that it receives? In other words, how does that code base implement input guarding? I think that there should be more to this function.
For instance, we should do more to ensure that if the user intended to use the 'ms' metric type, we can confirm that the input matched exactly what we expected (obviously, we would have to account for any leading and trailing white space characters and maybe a null terminating character).
Line 146: This function checks to see if the statistic name given to the StatsD
: * dialplan application is valid by ensuring that the name does not have any
: * invalid characters.
I know this is redundant, but please list out the invalid characters and don't assume any prior knowledge on the part of your reader.
E.g. "The only invalid character for the 'name' input is the pipe character (|)."
Also, this pattern is used again in validate_value for your set metric types. You can rename this function and reuse the pattern.
Line 153: static int validate_name(const char *name)
: {
: if (ast_strlen_zero(name) || (strstr(name, "|") != NULL)) {
: ast_log(AST_LOG_ERROR, "Statistic name %s is missing or contains a pipe (|)"
: " character.\n", name);
: return 1;
: }
:
: return 0;
: }
StatsD input parameter 'value' is contains a pipe (|) character.
To make this more generic, such that you can reuse it with the validate_value function consider revising to something like the following:
static int check_for_illegal_stuff(const char *value)
{
if (ast_strlen_zero(name)) {
ast_log(AST_LOG_ERROR,
"StatsD input parameter '%s' is missing.\n",
name,
value);
return 1;
}
if (ast_strlen_zero(name)) {
ast_log(AST_LOG_ERROR,
"StatsD input parameter '%s' contains a pipe character (|).\n",
name,
value);
return 1;
}
return 0;
}
Line 170: * This function checks to see if the value given to the StatsD daialplan
: * application is valid by testing if it is numeric. A plus or minus is only
: * allowed at the beginning of the value if it is a counter or a gauge. A set
: * can send a string as the value, but it cannot contain a pipe character.
This really needs to be reworded since you refactored it to include sets.
I would state it this way:
This function validates input values before sending the
statistic to the StatsD server according to the following
rules:
Metric Type Validation Rules
counters, gauges An optional leading
character of a plus (+) or
minus (-) followed by a
numeric value.
sets All characters except for
the pipe character (|),
which is not allowed.
Line 190: in a set metric.
Change this to:
for the 'set' metric type.
Line 200: "Value argument %s only contains a sign"
: " operator.\n"
Invalid value '%s' for StatsD parameter 'value': value must be a numeric value.
Yes, I am aware of the overuse of the word 'value' in the above statement :p. If you can rephrase that in a better way, go for it.
--
To view, visit https://gerrit.asterisk.org/1571
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I11d315d0a5034fffeae1178e650aa8264485ed52
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Tyler Cambron <tcambron at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Tyler Cambron <tcambron at digium.com>
Gerrit-HasComments: Yes
More information about the asterisk-code-review
mailing list