Dogstatsd mixer metrics adapter#3368
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
googlebot
added
the
cla: no
|
Hi @awwithro. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
I signed it! |
|
CLAs look good, thanks! |
googlebot
added
cla: yes
and removed
cla: no
|
@awwithro PR needs rebase |
istio-merge-robot
added
needs-rebase
|
/ok-to-test |
douglas-reid
left a comment
douglas-reid
left a comment
There was a problem hiding this comment.
Awesome. Thanks for contributing!
I had a few initial comments.
Gopkg.lock
Outdated
There was a problem hiding this comment.
looks like a merge issue.
There was a problem hiding this comment.
can we reuse the "default" metrics already defined instead of defining new ones (for example: requestcount.metric) ? These look nearly identical.
There was a problem hiding this comment.
Can we add some documentation here? This will form the basis of the website docs, so it would be good to have thorough docs here.
There was a problem hiding this comment.
Ah didn't realize that. I'll expand on the comments.
There was a problem hiding this comment.
nit: suggest naming this UNKNOWN_TYPE, per https://cloud.google.com/apis/design/naming_convention#enum_names
There was a problem hiding this comment.
why duplicate this locally?
There was a problem hiding this comment.
I'd seen it in another adapter and figured it was a test fixture of some sort, I'll clean it up
There was a problem hiding this comment.
Consider adding an example for how to write equivalent yaml config.
example https://github.com/istio/istio/blob/master/mixer/adapter/solarwinds/config/config.proto#L34
This comment will automatically show up on istio documentation page https://istio.io/docs/reference/config/adapters/solarwinds.html
There was a problem hiding this comment.
Is there a default value for this ? Consider adding what is the default for any field in this config, if there is one. Also, consider mentioning, which is required and which is optional
There was a problem hiding this comment.
curious: the values for these tags are constant literals right ?
There was a problem hiding this comment.
any reason this cannot be same as instance_name ?
There was a problem hiding this comment.
I find its useful to override the istio metric name to remove things like the "istio-system" namespace or to match a different naming standard for metrics in datadog
There was a problem hiding this comment.
consider commenting on what each of these mean
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
Similarly this validation of datatype of value can be done during Validate call. The SetMetricType gives exactly what is the datatype of the value field.
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
consider adding more context, what types are allowed for what kind of metrics.
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
this is default, please add to the comments of adapter config.
There was a problem hiding this comment.
Consider adding a real world example for global_tags user would want to add .
There was a problem hiding this comment.
consider adding an example for per metric tag in adapter config.
There was a problem hiding this comment.
Please add Istio documentation markers. Look in other adapters for the $location, $title, and $overview markers. See the Istio developer's guide for info on writing docs for protos.
Thanks.
There was a problem hiding this comment.
Please capitalize Mixer here and elsewhere.
There was a problem hiding this comment.
Could you explain in the comment why one would alter this value? Does Datadog need corresponding config updates when twiddling this value?
There was a problem hiding this comment.
I took another look at the client and it turns out this buffer is totally different than what I thought it was. I thought it was for adjusting the UDP packet size to fill before sending metrics (similar to the non-datadog statsd client) but instead its a buffer of individual metrics regardless of size. The buffer is flushed every 100ms regardless of size and can result in packet drops if the buffer is large.
Given this I think a default of non-buffering makes more sense but I'm torn on leaving this as a knob for tuning (with a warning) given that it could lead to undesired behavior. Thoughts?
There was a problem hiding this comment.
It would be preferable to generate when this happens. Otherwise, the operator could easily have data being dumped in the toilet and not know why. So just generate errors when things don't line up.
You can discover exactly what you will be handed at runtime during validation. So you can report errors to the user there, before receiving traffic.
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
No need for this test, the Validate call took care of it.
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
This is pretty expensive. Could this be done by calling .String on the object instead?
There was a problem hiding this comment.
I'd tried a type assertion initially but since not all dimensions are strings it panicked. I figured that the fmt package would be better than switch on each dimension type
There was a problem hiding this comment.
Sprintf is very expensive relatively speaking. The SolarWinds adapter's metric_handler.go has a fast path for common data which would be useful here.
@guptasu Sunny, Seems like we need to add a utility to pkg/adapter that returns a stringified version of all supported value types we support. We could scour all adapters and use as needed. The memquota adapter also needs to compare value types in a generic way, so maybe we need that too. WDYT?
There was a problem hiding this comment.
You are talking about matchDimension in memquota ?
@geeknoid, I agree, we should do everything possible to make lives easy for adapters + performant. This is a good example. I will send a PR soon, hopefully it can be used for this adapter.
There was a problem hiding this comment.
I think the redis quota adapter has similar logic. Both also contain a makeKey function which converts dimensions into strings which would benefit from a generic function for each value type.
There was a problem hiding this comment.
fwiw, the prometheus adapter has the same logic here as well:
istio/mixer/adapter/prometheus/prometheus.go
Line 340 in 4fe1287
There was a problem hiding this comment.
I'll wait for that commit to be merged to incorporate
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
Ensure this can't happen by validating the input config up front.
|
@awwithro PR needs rebase |
istio-merge-robot
added
the
needs-rebase
|
PTAL, I've incorporated changes around docs/validation config. I think that covers the primary asks other than the string formatting which I can add when the stringifier is merged |
geeknoid
left a comment
geeknoid
left a comment
There was a problem hiding this comment.
Looking good. Just a few comments.
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
Can just combine with next line as return nil, env.Logger.Errorf(...)
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
byt -> by
This check is already done in Validate, right?
The Mixer contract is that it won't call Build unless Validate succeeds.
There was a problem hiding this comment.
Validate checks that the adapterConfig metrics exist as a valid metric type while this is looking at the opposite. I was going to add this to validate but 1. didn't think it necessitates failing validation in this case and 2. validate doesn't have access to the env.Logger to generate a warning (unless I've missed something).
There was a problem hiding this comment.
You're right, the Validate function doesn't have access to an env logger. All validation errors should be returned in the error.
The idea with Validate is that anything wrong in the config entered by the operator should be reported there. This will prevent bad config from entering the system at all. By the time Build runs, the only errors we should be producing are the transient variety around network connectivity or credentials. Things that are strictly related to operator errors should be in Validate.
In general, we want to encourage config to be well-formed and strongly correct in order to eliminate ambiguities and issues with future upgradability. So we want to be aggressive about flagging errors.
There was a problem hiding this comment.
Stepping back a bit. When SetMetricsType gets called on the builder, are the metrics that are passed in all of the metric templates that the mixer knows about or just the metrics that have rules that map them to the adapter? My first thought was that it was all metrics and it made sense to me to log and continue in that case.
If SetMetricsType is only passing specific metrics to the handler and the handler doesn't have the corresponding config then, yes a validation error makes sense there.
|
It's a self-consistent model: SetMetricTypes is passed all metrics that
could be sent to the handler, and HandleMetric is only called for metrics
that were introduced with SetMetricTypes.
This lets you do all static validation in Validate, and the only errors
that emerge from HandleXXX methods are the usual runtime errors.
…On Tue, Feb 13, 2018 at 2:01 PM, Alex Withrow ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In mixer/adapter/dogstatsd/dogstatsd.go
<#3368 (comment)>:
> + if ac.BufferLength > 0 {
+ client, err = statsd.NewBuffered(ac.Address, int(ac.BufferLength))
+ } else {
+ client, err = statsd.New(ac.Address)
+ }
+ if err != nil {
+ env.Logger().Errorf("Unable to create dogstatsd client: %v", err)
+ return nil, err
+ }
+ client.Namespace = ac.Prefix
+ client.Tags = flattenTags(ac.GlobalTags)
+
+ // Generate a warning for metrics that the Mixer emits but aren't handled by the adapter
+ for mname := range b.metricTypes {
+ if _, found := ac.Metrics[mname]; !found {
+ env.Logger().Warningf("%s is a valid metric but is not configured to be handled byt the datadog adapter", mname)
Stepping back a bit. When SetMetricsType gets called on the builder, are
the metrics that are passed in all of the metric templates that the mixer
knows about or just the metrics that have rules that map them to the
adapter? My first thought was that it was all metrics and it made sense to
me to log and continue in that case.
If SetMetricsType is only passing specific metrics to the handler and the
handler doesn't have the corresponding config then, yes a validation error
makes sense there.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3368 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AVucHQxi5Pk99JGGc0Daca2KZxqWcd-3ks5tUgYzgaJpZM4SAZKy>
.
|
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
|
/lgtm |
guptasu
left a comment
guptasu
left a comment
There was a problem hiding this comment.
Minor comments. Almost LGTM
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
mixer/adapter/dogstatsd/dogstatsd.go
Outdated
There was a problem hiding this comment.
@awwithro you should now be able to replace this code with the new code from pkg/adapter.Stringify
There was a problem hiding this comment.
will do. For purposes of rebasing/pushing is the PR "good enough" to force push and lose comment history?
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
googlebot
added
cla: no
PR Feedback * Disable buffering by default * Validate statsd address * Warn on unhandled metrics * Update sample/docs Copy map to avoid a race condition
|
CLAs look good, thanks! |
googlebot
added
cla: yes
and removed
cla: no
|
@ldemailly PTAL I think this is all set |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geeknoid, guptasu, ldemailly The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
thx for going through the new vendor/ process successfully ! (my lgtm is only about the vendor/toml - I assume the rest is still same as before/approved by others) |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
@awwithro: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Thanks again for the help and getting this added to the code base. Biggest feedback would be to consolidate the vendor faq with the contribution repo. I liked the makefile approach for setting up the submodules and extending that to validate/warn the local dev env would be a good thing. The other thing is that I used the statsd/prometheus adapters as a reference but it looks like they predate much of code improvement/docs of later adapters. It'd be good to have some open issues to increase their quality. The label:"community/good first issue" would be a good fit for them. |
|
there are lint failures on master |
|
Hmm, not sure how those slipped in. |
8 participants
This PR adds a datadog adapter that sends metrics using the dogstatsd flavor of statsd. I modeled this largely using the statsd and prometheus adapters since dogstatsd "tags" are similar to prometheus "labels" applied to statsd metric names.
I have this adapted currently running in a staging environment of mine and can confirm it is working as intended. I've only implemented statsd type metrics with tags from dimensions. There is also the potential to emit checks/events to datadog using istio telemetry but that's out of scope for what I wanted to build for an initial adapter.
Please let me know how this looks or if I've missed anything important.