Feature/datadog #411

ivoreis · 2017-12-12T18:33:47Z

I have a use case for this tool but I need to integrate with our current StatsD report tool.

The goal of this PR is to create a simple Datadog integration and it's still a WIP.

DogStatsD implements the StatsD protocol (doc)

Based on the feedback the idea is to provide 2 new collectors:

Usage:

// STATSD_PORT, STATSD_BUFFER_SIZE can also be passed to override the defaults
STATSD_ADDR=address k6 run sample.js -o statsd

// DATADOG_PORT, DATADOG_BUFFER_SIZE, DATADOG_TAG_WHITELIST can also be passed to override the defaults
DATADOG_ADDR=address DATADOG_NAMESPACE=k6.myapp. k6 run sample.js -o datadog

Relates to #343, #196.

Feedback welcome :)

liclac · 2017-12-12T18:35:40Z

Hm, could you revert the core-js version bump there? I don't mind if you want me to update it/send a PR that does it, but it doesn't seem relevant to this PR.

liclac · 2017-12-12T19:34:02Z

My biggest question wrt the design in general: is this specific to Datadog, or is it general-purpose for any StatsD server? I see that Datadog extended the StatsD protocol a bit, but are those changes portable to other servers?

What I'd probably do is PR this as two collectors in one: one that just talks to a statsd server, one that wraps that one (with its own configuration/login) and talks specifically to datadog (hardcoding their statsd address, using their login system if any, etc.).

ivoreis · 2017-12-12T20:39:11Z

AFIK the major difference is that dogstatsd extends statsd and adds tags. If tags are sent to a generic statsd server the server will output bad line packet.

Taking the path of a double collector both implementations would be very similar varying only the ability to pass extra tags.

liclac · 2017-12-12T20:57:57Z

I'd like it a lot if you did that, and then changed the environment variables, etc. of this collector to be datadog-specific (defaulting to their server address!), that'll avoid a lot of confusion.

liclac · 2017-12-12T20:58:32Z

Of course, if you'd rather just contribute the datadog collector, that's fine too!

ivoreis · 2017-12-12T22:06:32Z

Sure I can do that. Currently they still use the same ENVs but in order to avoid confusion I'll split them out. Just gets a bit tricky whilst trying to avoid duplication because both collectors a roughly the same.

liclac · 2017-12-13T01:18:52Z

The cloud collector/executor (currently only on a feature branch) puts shared code in one place and the other is more like a skeleton that calls stuff from the other place, you could do something like that?

ivoreis · 2017-12-13T09:42:59Z

Yep I'm not happy with my current implementation either, I'll have a look how the cloud bit is handled.

liclac · 2017-12-15T17:26:02Z

Ah, core/... is for executors, eg. local, cloud, cluster, etc. Keep collectors in stats.

ivoreis · 2017-12-17T19:00:27Z

Sure I've made that change. Anything else you see that needs improvement?

liclac · 2017-12-18T21:28:03Z

Please don't change js/lib/core-js ^^;;

liclac · 2017-12-18T21:26:43Z

cmd/collectors.go

+	collectorJSON      = "json"
+	collectorCloud     = "cloud"
+	collectorStatsD    = "statsd"
+	collectorDogStatsD = "dogstatsd"


Hm, I kinda feel like "datadog" would be a more obvious name here.

I think that the only benefit of having as datadog is that it's a shorter name, other than that statsd and dogstatsd makes more sense in terms of convention because in practice it's an implementation of a statsd protocol. Both implement statsd protocal but dogstatsd extends it. I believe is more transparent that way.

What I'm thinking is that most people will come looking for "datadog" and instead find "dogstatsd", then get confused by what the difference is, while people who are running dogstatsd know what they're doing. I could be wrong.

That's a fair point. Do you think that can be solved with documentation?

Possibly, but I feel like it's better to avoid the confusion in the first place.

Unfortunately, a lot of people seem to really hate reading the effing manual.

@liclac I've updated the collector name to be datadog

liclac · 2017-12-18T21:29:16Z

stats/dogstatsd/collector.go

+		Config: conf,
+		Logger: log.WithField("type", statsd.DogStatsD.String()),
+		Type:   statsd.DogStatsD,
+		Tagger: tagHandler,


Wouldn't it be cleaner to do &TagHandler{} here?

I don't see it used anywhere else in this function...

Well spotted.

liclac · 2017-12-18T21:29:55Z

stats/dogstatsd/config.go

+import (
+	"encoding/json"
+
+	statsd "github.com/loadimpact/k6/stats/statsd/shared"


common as a package name would be more consistent with how the JS runner is organised.

I'll update

liclac · 2017-12-18T21:31:01Z

stats/statsd/collector.go

+		Config: conf,
+		Logger: log.WithField("type", statsd.StatsD.String()),
+		Type:   statsd.StatsD,
+		Tagger: tagHandler,


Same comment here, &TagHandler{} here seems cleaner?

liclac · 2017-12-18T21:33:48Z

stats/dogstatsd/config.go

+type Config ConfigFields
+
+// Address returns client address
+func (c Config) Address() string {


Hm, why are these accessors? They don't seem to do anything.

They are being used to normalise StatsD and DogStatsD config.
https://github.com/uswitch/k6/blob/cf72b91566cbd1dc886ad58ef0ff8f60c215f420/stats/statsd/common/config.go#L10

Couldn't you use the same struct and just ignore the fields one doesn't use?

I could but they refer to different ENV's. https://github.com/uswitch/k6/blob/cf72b91566cbd1dc886ad58ef0ff8f60c215f420/stats/statsd/config.go#L31 vs https://github.com/uswitch/k6/blob/cf72b91566cbd1dc886ad58ef0ff8f60c215f420/stats/dogstatsd/config.go#L31

Possible workaround:

// package shared type Config { Addr string `envconfig:"ADDR"` } // package statsd type Config struct { shared.Config `envconfig:"STATSD"` } // package dogstatsd type Config struct { shared.Config `envconfig:"DOGSTATSD"` }

hmm not sure about this. Just feels a bit weird and the interface solutions looks cleaner. What do you think the trade-off is here?

Your version is… really verbose, and it's essentially solving a parsing problem by adapting the data, which feels backwards to me.

You could also do something like just setting envconfig:"ADDR" and then using a different prefix in envconfig.Process().

I agree, is really verbose. I think using process is a good idea, let me give it a go.

liclac · 2017-12-18T21:35:45Z

stats/statsd/shared/collector.go

+
+// Run the collector
+func (c *Collector) Run(ctx context.Context) {
+	c.Logger.Debugf("%s: Running!", c.Type.String())


Use WithField()/WithFields() rather than Levelf().

Not sure what you mean here. Type is used to prefix the string not as a meta/tagging. I guess if that goes again't any project convention I'll have to change all of the other similar instances.

liclac · 2017-12-18T21:38:10Z

Getting there! The only big question I have is about the naming - datadog feels like a more obvious name than dogstatsd.

Also: consider implementing k6 login datadog and/or k6 login statsd commands while you're at it.

ivoreis · 2017-12-18T22:02:18Z

@liclac What do you mean with k6 login datadog and statsd? There's no need to authenticate with statsd / dogstatsd.

liclac · 2017-12-18T22:32:01Z

Huh, how do you push metrics to datadog in that case? (I've never used it before ^^;;)

ivoreis · 2017-12-18T22:41:21Z

You just need an endpoint and voila :) Ref and doc. Normally you an agent running and you send data to it. The client doesn't authenticate, just the agent.

liclac · 2017-12-18T22:45:53Z

Ah! That makes sense.

Maybe you could just add a login command that lets you specify a default endpoint? Because what saved credentials essentially do is just let you store default configs for when you just do -o [type] with no arguments - -o influxdb isn't too bad, typing -o influxdb=http://localhost:8086/k6 over and over is just annoying.

ivoreis · 2017-12-18T22:52:06Z

That's a good point. In the case of passing an url I guess passing addr and port becomes redundant. What's the difference between constructing the url viaADD+PORT and args from command influxdb=... Is there any preferable option?

ivoreis · 2017-12-19T11:48:17Z

cmd/collectors.go

@@ -56,8 +56,8 @@ func parseCollector(s string) (t, arg string) {
 }

 func newCollector(t, arg string, src *lib.SourceData, conf Config) (lib.Collector, error) {
-	loadConfig := func(out encoding.TextUnmarshaler) error {
-		if err := envconfig.Process("k6", out); err != nil {
+	loadConfig := func(out encoding.TextUnmarshaler, namespace string) error {


I added an extra arg to loadConfig func. What are the use cases for K6 env var?

What do you mean?

I've extracted the hard-coded k6 string and made it a var that is set when the config is loaded. The question was to understand what are the use cases for k6 usage and if we would need to provide a k6+statsd / k6+datadog combination.

Ah, what I answered in my other comment?

I'm sorry, I'm not sure what the question is here... ^^;;

^^:: No worries. Probably I'm just overcomplicating things. Are you happy with this change?

Definitely!

Honestly, at this point you could just change the loadConfig calls to envconfig.Process calls wholesale. That function basically existed to not have to type (and potentially typo "k6") all the time.

I agree but I tried to keep it consistent to how was done before. Happy to do other PR to simplify loadConfig stuff.

liclac · 2017-12-19T21:37:55Z

Optimally, people should be able to use whichever one they prefer, and possibly whichever works better in the context they're working in.

On my laptop, I have one local InfluxDB server, so I want to just be able to do k6 run -o influxdb ...; but if I was running on, say, Kubernetes, I'd probably want to have a password-protected influxdb server and use Secrets, which can be easily bound to environment variables (K6_INFLUXDB_USERNAME, K6_INFLUXDB_PASSWORD, ...).

stats/datadog/collector.go

liclac · 2017-12-20T23:14:08Z

stats/statsd/common/api.go

@@ -0,0 +1,30 @@
+package statsd


This package should be called common.

StatsD - Basic statsd functionality (Count / TimeInMiliseconds / Gauge) DogStatsD - The same as StatsD + ability to send Tags

core/statsd contains a generic statsd implementation that stats/dogstatsd and stats/statsd extends. The collector implementation only contains the config for itself and the method to generate a new Container instance.

This reverts commit 3b43b6c.

accepted shape is ADDR:PORT

codecov-io · 2017-12-27T15:25:37Z

Codecov Report

Merging #411 into master will decrease coverage by 0.08%.
The diff coverage is 12.5%.

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   62.52%   62.43%   -0.09%     
==========================================
  Files          89       89              
  Lines        6234     6246      +12     
==========================================
+ Hits         3898     3900       +2     
- Misses       2122     2132      +10     
  Partials      214      214

Impacted Files	Coverage Δ
cmd/collectors.go	`0% <0%> (ø)`	⬆️
cmd/config.go	`35.48% <100%> (+2.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d50aad...3b587a4. Read the comment docs.

robingustafsson · 2018-01-31T14:30:08Z

@ivoreis I'm going to have a look at this. What is still WIP so I know when testing?

mstoykov · 2019-01-11T07:53:48Z

closed in favour of #893

liclac suggested changes Dec 18, 2017

View reviewed changes

ivoreis commented Dec 19, 2017

View reviewed changes

liclac suggested changes Dec 20, 2017

View reviewed changes

ivoreis added 9 commits December 27, 2017 15:04

Initial spike to add datadog / statsd integration

85cefc7

Add datadog deps

5c71fe4

Use env to create statsd connection string

0335e94

Set default buf size to 10

1c22d53

Add default statsd port

0d63325

Revert js/lib/corejs

3dc1729

Make name tag consistent

d8c6a05

Make log consistent, use debug instead of info

e9a24d5

Flush metrics to statsd

7aad10e

ivoreis added 15 commits December 27, 2017 15:04

Add statsd tag when logging

22ac688

Split datadog collector into 2 separate ones

339bd52

StatsD - Basic statsd functionality (Count / TimeInMiliseconds / Gauge) DogStatsD - The same as StatsD + ability to send Tags

StatsD and DogStatsD have different ENVs

4343017

Skip error check. This would be raised by Flush method

8eed132

Split common stats chunks into core/statsd

256ad8f

core/statsd contains a generic statsd implementation that stats/dogstatsd and stats/statsd extends. The collector implementation only contains the config for itself and the method to generate a new Container instance.

Move statsd shared files from core to stats

f35e7e8

Cleanup summary

24d6800

Pass duration to summary

9deb553

Refactor check label to string

4cd52ee

Revert "Revert js/lib/corejs"

2005032

This reverts commit 3b43b6c.

Apply PR requested changes

c074226

Rename dogstatsd to datadog

92107c2

Simplify shared config

19ff58e

Add the ability to pass connection string via -o

ba07cff

accepted shape is ADDR:PORT

Update namespace and add copyright notices

3b587a4

ivoreis mentioned this pull request Jan 10, 2019

Add datadog / statsd integration #893

Closed

mstoykov closed this Jan 11, 2019

Feature/datadog #411

Feature/datadog #411

Conversation

ivoreis commented Dec 12, 2017 • edited Loading

liclac commented Dec 12, 2017

liclac commented Dec 12, 2017

ivoreis commented Dec 12, 2017

liclac commented Dec 12, 2017

liclac commented Dec 12, 2017

ivoreis commented Dec 12, 2017 • edited Loading

liclac commented Dec 13, 2017

ivoreis commented Dec 13, 2017

liclac commented Dec 15, 2017

ivoreis commented Dec 17, 2017

liclac commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoreis Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liclac commented Dec 18, 2017

ivoreis commented Dec 18, 2017

liclac commented Dec 18, 2017

ivoreis commented Dec 18, 2017 • edited Loading

liclac commented Dec 18, 2017

ivoreis commented Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liclac commented Dec 19, 2017

Choose a reason for hiding this comment

codecov-io commented Dec 27, 2017

Codecov Report

robingustafsson commented Jan 31, 2018

mstoykov commented Jan 11, 2019 • edited Loading

ivoreis commented Dec 12, 2017 •

edited

Loading

ivoreis commented Dec 12, 2017 •

edited

Loading

ivoreis Dec 18, 2017 •

edited

Loading

ivoreis commented Dec 18, 2017 •

edited

Loading

ivoreis commented Dec 18, 2017 •

edited

Loading

mstoykov commented Jan 11, 2019 •

edited

Loading