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

Support dogstatd with tags #165

Closed
killcity opened this issue Sep 27, 2016 · 31 comments · Fixed by #476
Closed

Support dogstatd with tags #165

killcity opened this issue Sep 27, 2016 · 31 comments · Fixed by #476

Comments

@killcity
Copy link

Will Fabio eventually support (or does it already?) tagging statsd metrics? This is key when using Datadog.

@magiconair
Copy link
Contributor

It may but I don't know. Someone else implemented the statsd support. Can you give an example ?

@killcity
Copy link
Author

killcity commented Sep 27, 2016

In it's simplest form:

https://help.datadoghq.com/hc/en-us/articles/206441345-Send-metrics-and-events-using-dogstatsd-and-the-shell

I also found a dogstatsd client written in Go.

https://github.com/ooyala/go-dogstatsd

Instead of having a bunch of metrics named explicitly per consul agent, we would have them sending with the same metric name and use tags to identify host, etc.

@magiconair
Copy link
Contributor

Is that compatible with the standard statsd client?

@killcity
Copy link
Author

killcity commented Sep 27, 2016

I found one written by Datadog themselves.

https://github.com/DataDog/datadog-go/tree/master/statsd https://github.com/DataDog/datadog-go/tree/master/statsd

I’m pretty certain this supports traditional statsd as well as extended tagging support for Dogstatsd users. I will confirm with Datadog.

@killcity
Copy link
Author

To my chagrin, it doesn't support standard statsd. The datagram format is different.

http://docs.datadoghq.com/guides/dogstatsd/#datagram-format

I'm discussing some plausible solutions with a couple other engineers.

@magiconair
Copy link
Contributor

If they are substantially different then we can just add it as another metric provider. If they were compatible then it could have made sense to just keep one of the two. You want to try a PR or want me to work on this?

@killcity
Copy link
Author

killcity commented Sep 27, 2016

The reporter you might be looking for?

Both of these look like they woud do the job: https://github.com/ooyala/go-dogstatsd and https://github.com/DataDog/datadog-go .

I'm a hack of a Go dev :) Perhaps if you have the cycles you can try? Thanks again for all the hard work on this project. Looking forward to pushing some serious traffic at a tier of Fabio LBs!

@magiconair magiconair changed the title Tagging support for Dogstatsd Support dogstatd with tags Sep 27, 2016
@magiconair
Copy link
Contributor

ok, but then you'll have to wait a couple of days since I'm reworking the the parser for the configuration to support the path stripping that people seem to really want - besides other things.

@killcity
Copy link
Author

That sounds fantastic. Thanks!

@aporcano
Copy link

Hey folks, is there any news on this functionality?

@magiconair
Copy link
Contributor

Not yet. Just back from vacation and digging through the backlog.

@jippi
Copy link

jippi commented Jan 8, 2017

Any love for this? :D

@magiconair
Copy link
Contributor

magiconair commented Jan 8, 2017 via email

@jippi
Copy link

jippi commented Jan 8, 2017

@magiconair Yep, I can test! :)

@killcity
Copy link
Author

killcity commented Jan 8, 2017 via email

@magiconair
Copy link
Contributor

magiconair commented Jan 9, 2017

Looks like there is an open issue on the go-metrics library to add tags support: rcrowley/go-metrics#135 but that isn't going anywhere.

We need to define an option to define the tags per route. What about mtags=foo=1;bar or metricsTags=foo=1;bar?

Since the go-metrics lib does not support tags natively at this point the best approach is to encode the tags in the key name (e.g. key#tag1;tag2) and then parse them out again before reporting the metrics. That would require forking the statsd driver so that it understands the key format.

@magiconair
Copy link
Contributor

Right now I am assuming that the tags are static per route. Is that a valid assumption?

@jippi
Copy link

jippi commented Jan 9, 2017

I've written up a small statsd proxy that can convert fabio statsd path into tagged metrics; https://github.com/bownty/statsd-rewrite-proxy :)

@jippi
Copy link

jippi commented Jan 9, 2017

@jippi
Copy link

jippi commented Jan 9, 2017

tagged metrics is a bit tricky, since they also require the actual statsd path name to change, for it to make sense within UIs like datadog

update

not sure if its relevant in this project or upstream somewhere, but while writing the tool I noticed the following (invalid) metric being exposed

fabio.--admin-demo.example.com./.192.168.1.100_38473.mean:%!|(float64=0)gf`  

looks like the float is badly encoded

@magiconair
Copy link
Contributor

@jippi This is #207.

Can you explain your first statement a bit more? I'm not sure I get it. I thought I just add the #tag1;tag2 to the statsd format and that's it.

@magiconair
Copy link
Contributor

@jippi Created pubnub/go-metrics-statsd#1 to fix #207

magiconair added a commit that referenced this issue Jan 9, 2017
This code is a proof-of-concept to implement statsd
metrics with tags which work on dogstatd.

This is not the final code and will change before
being merged.
@magiconair
Copy link
Contributor

@jippi and @killcity I've pushed a proof-of-concept into the branch which hopefully does what you want. You need to add mtags=tag1;tag2 to your urlprefix-/foo definitions so that they look like urlprefix-/foo mtags=tag1;tag2.

This is not the final code since I've hacked this into the pubnub/statsd implementation. I'd just like to check that I got the format right and that it does what it should do. Could you have a look please?

@jippi
Copy link

jippi commented Jan 10, 2017

@magiconair for tagged metrics to be useful inside datadog UI, the path need to be "tag" less.

e.g. fabio.http.status.{code} make sense to emit as fabio.http.response_code

then in the UI you can select fabio.http.response_code in the UI and group by the {code} (as its emitted as a named tag code:200 example, and use these tags as facets.

So for me, tagged metrics is about a new path structure where the "value" from the normal statsd path is moved into named tags code:200 or service:example-service - that structure works really really well inside datadog - datadog docs; http://docs.datadoghq.com/guides/metrics/#tags

@magiconair
Copy link
Contributor

magiconair commented Jan 10, 2017 via email

magiconair added a commit that referenced this issue Jan 17, 2017
This code is a proof-of-concept to implement statsd
metrics with tags which work on dogstatd.

This is not the final code and will change before
being merged.
@killcity
Copy link
Author

killcity commented Feb 2, 2017

I think we would want to auto publish the following tags (which would be used in conjuction with the paths that appear in Datadog):

service:
code:
urlprefix:

and the following paths:

fabio.http.status.code
fabio.http.request.count
fabio.http.request.min
fabio.http.request.max

Can you guys think of any others? Does this make sense? One thing I'm not totally sure of is if we want to use:

fabio.http.status.code

We would need to auto publish all the various response codes along with the ones listed above sans "code:".

@magiconair
Copy link
Contributor

@killcity so you're also in favor of pushing the status code as a tag instead of the metric name just like @jippi suggested?

The main reason the change takes a bit longer is that the go-metrics library doesn't natively support tags which means that depending on the metrics backend fabio needs to report different metric names and put values into different places (e.g. service name, urlprefix, status code, ...)

I'm mulling over adding a full metrics abstraction layer which handles this or add a handful if dogstatdMetrics statements. Not so clean but much less work. I can do the nice refactor later.

@killcity
Copy link
Author

Sorry for the long delay. Yes, I am also in favor of pushing the status code as a separate tag. Thanks for working on this one.

@magiconair
Copy link
Contributor

Since 1.4 is out and the TCP proxy stuff is released I think I'll pick this one up again next together with the other metrics related issues #211, #253, #126

@magiconair magiconair added this to the 1.6.0 milestone Oct 10, 2017
magiconair added a commit that referenced this issue Mar 24, 2018
This patch refactors the metrics support within fabio to
achieve several goals:

 * support labels for DogstatD, Prometheus and others
 * support raw events for statsd and others without aggregation
 * support multiple backends simultaneously to support migration
 * make integration of new metrics backends and/or libraries simple
 * being fully backwards compatible to not break existing setups

One of the main challenges is that fabio generates names for the metrics
targets through templates and that these templates are configurable. A
metrics backend which supports labels needs to generate a different name
for the same metric than one that does not support labels.

Combining this with the need to support multiple different metrics
backends at runtime means that none of the existing metrics libraries
like go-kit/metrics and hashicorp/go-metrics is a good fit. However,
they can be used as drivers which reduces the need for testing and makes
integration of new backends simpler since fabio does not need to rely on
a single metrics library.

This patch is work in progress with the new metrics code in the metrics4
package and the old metrics code in the 'metrics_old' package where it
is kept for reference until it has been migrated or removed. The basic
architecture should be good enough to add more providers and
functionality so that the community can help.

Right now there are two providers "flat" and "label" to demonstrate the
concepts. They provide counter and timers in statsd/dogstatd format and
can be configured with -metrics.target=flat[,label]. Other providers
should be added in the same way.

The metrics_old code should be converted to the new provider scheme.

The go-kit/metrics library currently supports most of the necessary
drivers and is the preferred way of integrating new drivers.

The list below is a probably incomplete list of things that need to be
done:

 * The configuration is not used except for metrics.target
 * The drivers from the metrics_old package need to be migrated
 * Additional drivers from go-kit need to be added
 * The old rcrowley/go-metrics code relies on 'registries' for
   namespacing. After the routing table has changed the registry needs
   to be pruned of services which no longer exist so that go-metrics
   stops reporting them.
 * The RX/TX counters in the TCP and TCP+SNI proxy are probably
   sub-optimal or broken.
 * Some counters may not have been initialized properly, e.g. WSConn
 * WSConn should probably be a gauge which also needs to be set to 0 on
   startup.
 * The approach of injecting a noop metrics provider if none has been
   set has already bitten me once since some metrics are reported while
   others are not. I need to think about this a bit more.

Fixes #126
Fixes #165
Fixes #211
Fixes #253
Fixes #326
Fixes #327
Fixes #371
Closes #329
Closes #331
Closes #334
Closes #335
@magiconair
Copy link
Contributor

Please see #211 (comment)

@ketzacoatl
Copy link

@leprechau This is another metrics-related topic that would be good to review, as datadog is quite popular and the metrics integration is already possible via the existing statsd interface.

nathanejohnson pushed a commit that referenced this issue Oct 23, 2020
This patch refactors the metrics support within fabio to
achieve several goals:

 * support labels for DogstatD, Prometheus and others
 * support raw events for statsd and others without aggregation
 * support multiple backends simultaneously to support migration
 * make integration of new metrics backends and/or libraries simple
 * being fully backwards compatible to not break existing setups

One of the main challenges is that fabio generates names for the metrics
targets through templates and that these templates are configurable. A
metrics backend which supports labels needs to generate a different name
for the same metric than one that does not support labels.

Combining this with the need to support multiple different metrics
backends at runtime means that none of the existing metrics libraries
like go-kit/metrics and hashicorp/go-metrics is a good fit. However,
they can be used as drivers which reduces the need for testing and makes
integration of new backends simpler since fabio does not need to rely on
a single metrics library.

This patch is work in progress with the new metrics code in the metrics4
package and the old metrics code in the 'metrics_old' package where it
is kept for reference until it has been migrated or removed. The basic
architecture should be good enough to add more providers and
functionality so that the community can help.

Right now there are two providers "flat" and "label" to demonstrate the
concepts. They provide counter and timers in statsd/dogstatd format and
can be configured with -metrics.target=flat[,label]. Other providers
should be added in the same way.

The metrics_old code should be converted to the new provider scheme.

The go-kit/metrics library currently supports most of the necessary
drivers and is the preferred way of integrating new drivers.

The list below is a probably incomplete list of things that need to be
done:

 * The configuration is not used except for metrics.target
 * The drivers from the metrics_old package need to be migrated
 * Additional drivers from go-kit need to be added
 * The old rcrowley/go-metrics code relies on 'registries' for
   namespacing. After the routing table has changed the registry needs
   to be pruned of services which no longer exist so that go-metrics
   stops reporting them.
 * The RX/TX counters in the TCP and TCP+SNI proxy are probably
   sub-optimal or broken.
 * Some counters may not have been initialized properly, e.g. WSConn
 * WSConn should probably be a gauge which also needs to be set to 0 on
   startup.
 * The approach of injecting a noop metrics provider if none has been
   set has already bitten me once since some metrics are reported while
   others are not. I need to think about this a bit more.

Fixes #126
Fixes #165
Fixes #211
Fixes #253
Fixes #326
Fixes #327
Fixes #371
Closes #329
Closes #331
Closes #334
Closes #335

Remove 'Register' methods and create objects directly.

Add Gauge and use it for websocket connections

Vendoring in github.com/alexcesaro/statsd

First stab at integrating raw statsd from #335

Rebase off master 10/23/2020 - NJ
nathanejohnson pushed a commit that referenced this issue Oct 23, 2020
This patch refactors the metrics support within fabio to
achieve several goals:

 * support labels for DogstatD, Prometheus and others
 * support raw events for statsd and others without aggregation
 * support multiple backends simultaneously to support migration
 * make integration of new metrics backends and/or libraries simple
 * being fully backwards compatible to not break existing setups

One of the main challenges is that fabio generates names for the metrics
targets through templates and that these templates are configurable. A
metrics backend which supports labels needs to generate a different name
for the same metric than one that does not support labels.

Combining this with the need to support multiple different metrics
backends at runtime means that none of the existing metrics libraries
like go-kit/metrics and hashicorp/go-metrics is a good fit. However,
they can be used as drivers which reduces the need for testing and makes
integration of new backends simpler since fabio does not need to rely on
a single metrics library.

This patch is work in progress with the new metrics code in the metrics4
package and the old metrics code in the 'metrics_old' package where it
is kept for reference until it has been migrated or removed. The basic
architecture should be good enough to add more providers and
functionality so that the community can help.

Right now there are two providers "flat" and "label" to demonstrate the
concepts. They provide counter and timers in statsd/dogstatd format and
can be configured with -metrics.target=flat[,label]. Other providers
should be added in the same way.

The metrics_old code should be converted to the new provider scheme.

The go-kit/metrics library currently supports most of the necessary
drivers and is the preferred way of integrating new drivers.

The list below is a probably incomplete list of things that need to be
done:

 * The configuration is not used except for metrics.target
 * The drivers from the metrics_old package need to be migrated
 * Additional drivers from go-kit need to be added
 * The old rcrowley/go-metrics code relies on 'registries' for
   namespacing. After the routing table has changed the registry needs
   to be pruned of services which no longer exist so that go-metrics
   stops reporting them.
 * The RX/TX counters in the TCP and TCP+SNI proxy are probably
   sub-optimal or broken.
 * Some counters may not have been initialized properly, e.g. WSConn
 * WSConn should probably be a gauge which also needs to be set to 0 on
   startup.
 * The approach of injecting a noop metrics provider if none has been
   set has already bitten me once since some metrics are reported while
   others are not. I need to think about this a bit more.

Fixes #126
Fixes #165
Fixes #211
Fixes #253
Fixes #326
Fixes #327
Fixes #371
Closes #329
Closes #331
Closes #334
Closes #335

Remove 'Register' methods and create objects directly.

Add Gauge and use it for websocket connections

Vendoring in github.com/alexcesaro/statsd

First stab at integrating raw statsd from #335

Rebase off master 10/23/2020 - NJ
nathanejohnson pushed a commit that referenced this issue Oct 23, 2020
This patch refactors the metrics support within fabio to
achieve several goals:

 * support labels for DogstatD, Prometheus and others
 * support raw events for statsd and others without aggregation
 * support multiple backends simultaneously to support migration
 * make integration of new metrics backends and/or libraries simple
 * being fully backwards compatible to not break existing setups

One of the main challenges is that fabio generates names for the metrics
targets through templates and that these templates are configurable. A
metrics backend which supports labels needs to generate a different name
for the same metric than one that does not support labels.

Combining this with the need to support multiple different metrics
backends at runtime means that none of the existing metrics libraries
like go-kit/metrics and hashicorp/go-metrics is a good fit. However,
they can be used as drivers which reduces the need for testing and makes
integration of new backends simpler since fabio does not need to rely on
a single metrics library.

This patch is work in progress with the new metrics code in the metrics4
package and the old metrics code in the 'metrics_old' package where it
is kept for reference until it has been migrated or removed. The basic
architecture should be good enough to add more providers and
functionality so that the community can help.

Right now there are two providers "flat" and "label" to demonstrate the
concepts. They provide counter and timers in statsd/dogstatd format and
can be configured with -metrics.target=flat[,label]. Other providers
should be added in the same way.

The metrics_old code should be converted to the new provider scheme.

The go-kit/metrics library currently supports most of the necessary
drivers and is the preferred way of integrating new drivers.

The list below is a probably incomplete list of things that need to be
done:

 * The configuration is not used except for metrics.target
 * The drivers from the metrics_old package need to be migrated
 * Additional drivers from go-kit need to be added
 * The old rcrowley/go-metrics code relies on 'registries' for
   namespacing. After the routing table has changed the registry needs
   to be pruned of services which no longer exist so that go-metrics
   stops reporting them.
 * The RX/TX counters in the TCP and TCP+SNI proxy are probably
   sub-optimal or broken.
 * Some counters may not have been initialized properly, e.g. WSConn
 * WSConn should probably be a gauge which also needs to be set to 0 on
   startup.
 * The approach of injecting a noop metrics provider if none has been
   set has already bitten me once since some metrics are reported while
   others are not. I need to think about this a bit more.

Fixes #126
Fixes #165
Fixes #211
Fixes #253
Fixes #326
Fixes #327
Fixes #371
Closes #329
Closes #331
Closes #334
Closes #335

Remove 'Register' methods and create objects directly.

Add Gauge and use it for websocket connections

Vendoring in github.com/alexcesaro/statsd

First stab at integrating raw statsd from #335

Rebase off master 10/23/2020 - NJ
nathanejohnson pushed a commit that referenced this issue Dec 1, 2020
This patch refactors the metrics support within fabio to
achieve several goals:

 * support labels for DogstatD, Prometheus and others
 * support raw events for statsd and others without aggregation
 * support multiple backends simultaneously to support migration
 * make integration of new metrics backends and/or libraries simple
 * being fully backwards compatible to not break existing setups

One of the main challenges is that fabio generates names for the metrics
targets through templates and that these templates are configurable. A
metrics backend which supports labels needs to generate a different name
for the same metric than one that does not support labels.

Combining this with the need to support multiple different metrics
backends at runtime means that none of the existing metrics libraries
like go-kit/metrics and hashicorp/go-metrics is a good fit. However,
they can be used as drivers which reduces the need for testing and makes
integration of new backends simpler since fabio does not need to rely on
a single metrics library.

This patch is work in progress with the new metrics code in the metrics4
package and the old metrics code in the 'metrics_old' package where it
is kept for reference until it has been migrated or removed. The basic
architecture should be good enough to add more providers and
functionality so that the community can help.

Right now there are two providers "flat" and "label" to demonstrate the
concepts. They provide counter and timers in statsd/dogstatd format and
can be configured with -metrics.target=flat[,label]. Other providers
should be added in the same way.

The metrics_old code should be converted to the new provider scheme.

The go-kit/metrics library currently supports most of the necessary
drivers and is the preferred way of integrating new drivers.

The list below is a probably incomplete list of things that need to be
done:

 * The configuration is not used except for metrics.target
 * The drivers from the metrics_old package need to be migrated
 * Additional drivers from go-kit need to be added
 * The old rcrowley/go-metrics code relies on 'registries' for
   namespacing. After the routing table has changed the registry needs
   to be pruned of services which no longer exist so that go-metrics
   stops reporting them.
 * The RX/TX counters in the TCP and TCP+SNI proxy are probably
   sub-optimal or broken.
 * Some counters may not have been initialized properly, e.g. WSConn
 * WSConn should probably be a gauge which also needs to be set to 0 on
   startup.
 * The approach of injecting a noop metrics provider if none has been
   set has already bitten me once since some metrics are reported while
   others are not. I need to think about this a bit more.

Fixes #126
Fixes #165
Fixes #211
Fixes #253
Fixes #326
Fixes #327
Fixes #371
Closes #329
Closes #331
Closes #334
Closes #335

Remove 'Register' methods and create objects directly.

Add Gauge and use it for websocket connections

Vendoring in github.com/alexcesaro/statsd

First stab at integrating raw statsd from #335

Rebase off master 10/23/2020 - NJ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants