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

DataDog and StatsD Metrics Support #1701

Merged
merged 9 commits into from
Jul 20, 2017
Merged

DataDog and StatsD Metrics Support #1701

merged 9 commits into from
Jul 20, 2017

Conversation

aantono
Copy link
Contributor

@aantono aantono commented Jun 2, 2017

While still waiting for #1504 to be merged and rebase on top, this is to get the review process for #834 started for DataDog and StatsD metrics support.

The solution is based on the same go-kit/metrics library as for Prometheus, except using the DataDog and StatsD providers. Given that there is a possible integration from DataDog being created by DataDog/integrations-extras#64, I've used the matching metric names to push the metrics to DD agent, as the Prometheus pull is mapping. Internally the DD agent puller is still pushing the collected data into DogStatsD listener, so it will end up flowing though the same ingestion gate, thus taking advantage of the "included integration metrics".

I've had to add the additional dependencies into Glide, so after running ./scripts/glide.sh add <> it has changed a bunch of files in the vendor, so hopefully that's done correctly.

@aantono
Copy link
Contributor Author

aantono commented Jun 7, 2017

Rebased on top of #1504 changes from Master.

udp.ShouldReceiveAll(t, expected, func() {
n.ServeHTTP(recorder, req1)
n.ServeHTTP(recorder, req2)
// body := recorder.Body.String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code?

})
n.UseHandler(r)

req1, err := http.NewRequest("GET", "http://localhost:3000/ok", nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req1 := testhelpers.MustNewRequest(http.methodGet, "http://localhost:3000/ok", nil)

if err != nil {
t.Error(err)
}
req2, err := http.NewRequest("GET", "http://localhost:3000/not-found", nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req2 := testhelpers.MustNewRequest(http.methodGet, "http://localhost:3000/not-found", nil)


n.Use(metricsMiddlewareBackend)
r := http.NewServeMux()
//r.Handle("/metrics", promhttp.Handler())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead Code ?

}
pushInterval, err := time.ParseDuration(config.PushInterval)
if err != nil {
pushInterval = 10 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can log the error.

if err != nil {
t.Error(err)
}
req2, err := http.NewRequest("GET", "http://localhost:3000/not-found", nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req2 := testhelpers.MustNewRequest(http.methodGet, "http://localhost:3000/not-found", nil)

udp.ShouldReceiveAll(t, expected, func() {
n.ServeHTTP(recorder, req1)
n.ServeHTTP(recorder, req2)
// body := recorder.Body.String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code ?

glide.lock Outdated
subpackages:
- log
- swagger
- name: github.com/fatih/color
version: 9131ab34cf20d2f6d83fdc67168a5430d1c7dc23
- name: github.com/gambol99/go-marathon
version: 15ea23e360abb8b25071e677aed344f31838e403
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add/update only your dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very sure why this shows up as a diff, the glide.yaml has d672c6fbb499596869d95146a26e7d0746c06c54 version for github.com/gambol99/go-marathon which came from a prior commit (https://github.com/containous/traefik/blame/master/glide.yaml#L96)

Copy link
Contributor

@ldez ldez Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not only for go-marathon. It's a side affect of the glide get.
You must use a workaround:

  1. Revert the glide manifest and lock files.
  2. Use the github.com/multiplay/glide-pin plugin to pin all versions in the manifest file to the ones given in the lock file.
  3. Add your dependencies glide get
  4. Run glide update -v to update the lock file.
  5. Undo the pinning.
  6. Add your dependencies in glide.yml
  7. Use glide-hash to get the manifest file hash and put it in the lock file to please validate-glide.

Look at #1332 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to follow those steps, only to run into errors :(

Invoked glide pin followed by glide get github.com/go-kit/kit/metrics/dogstatsd commands and things exploded:

[ERROR]	Error scanning github.com/docker/docker/cli/config: open /Users/aantonov/.glide/cache/src/https-github.com-docker-docker/cli/config: no such file or directory
[ERROR]	This error means the referenced package was not found.
[ERROR]	Missing file or directory errors usually occur when multiple packages
[ERROR]	share a common dependency and the first reference encountered by the scanner
[ERROR]	sets the version to one that does not contain a subpackage needed required
[ERROR]	by another package that uses the shared dependency. Try setting a
[ERROR]	version in your glide.yaml that works for all packages that share this
[ERROR]	dependency.
[ERROR]	Error scanning github.com/docker/docker/cli/config/configfile: open /Users/aantonov/.glide/cache/src/https-github.com-docker-docker/cli/config/configfile: no such file or directory
[ERROR]	This error means the referenced package was not found.
[ERROR]	Missing file or directory errors usually occur when multiple packages
[ERROR]	share a common dependency and the first reference encountered by the scanner
[ERROR]	sets the version to one that does not contain a subpackage needed required
[ERROR]	by another package that uses the shared dependency. Try setting a
[ERROR]	version in your glide.yaml that works for all packages that share this
[ERROR]	dependency.
[INFO]	--> Fetching updates for github.com/xeipuuv/gojsonpointer.
[ERROR]	Error scanning github.com/docker/docker/cli/command/image/build: open /Users/aantonov/.glide/cache/src/https-github.com-docker-docker/cli/command/image/build: no such file or directory
[ERROR]	This error means the referenced package was not found.
[ERROR]	Missing file or directory errors usually occur when multiple packages
[ERROR]	share a common dependency and the first reference encountered by the scanner
[ERROR]	sets the version to one that does not contain a subpackage needed required
[ERROR]	by another package that uses the shared dependency. Try setting a
[ERROR]	version in your glide.yaml that works for all packages that share this
[ERROR]	dependency.
[INFO]	--> Fetching updates for golang.org/x/time.
[INFO]	--> Setting version for golang.org/x/time to a4bde12657593d5e90d0533a3e4fd95e635124cb.
[INFO]	--> Fetching updates for github.com/Azure/go-ansiterm.
[INFO]	--> Fetching updates for github.com/gorilla/mux.
[INFO]	--> Setting version for github.com/gorilla/mux to e444e69cbd2e2e3e0749a2f3c717cec491552bbf.
[INFO]	--> Fetching updates for github.com/vbatts/tar-split.
[INFO]	--> Fetching updates for github.com/opencontainers/runtime-spec.
[ERROR]	Failed to retrieve a list of test dependencies: Error resolving imports

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try two things:

  • Run glide cc first thing. (This will clear the glide cache which is sometimes known to cause problems.)
  • use script/glide.sh <command> instead of glide <command> everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, it becomes more and more likely that we are really running into the problem the glide error describes: conflicting dependencies.

I can double check on my end. Could you tell me which additional dependencies the PR needs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to add:

github.com/go-kit/kit/metrics/dogstatsd
github.com/go-kit/kit/metrics/statsd
github.com/go-kit/kit/metrics/multi
github.com/go-kit/kit/log
github.com/go-kit/kit/util/conn
github.com/stvp/go-udp-testing

Would greatly appreciate if you could see if it will work on your end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I ran into similar problem: Trying to upgrade github.com/stvp/go-udp-testing alone yields many conflicts with the client-go library.

I'm having a hard time understanding why we run into these conflicts since go-udp-testing looks super simple and seemingly doesn't have any direct or transitive dependencies with go-client.

Maybe we're holding it wrong at some point...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aantono could you squash all commits and remove changes on glide.yml, glide.lock and vendor? I will try to resolve the problem after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

histograms = append(histograms, m.getReqDurationHistogram())
retryCounters = append(retryCounters, m.getRetryCounter())
}
var mm MultiMetrics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this line after the next empty line.

udp.ShouldReceiveAll(t, expected, func() {
n.ServeHTTP(recorder, req1)
n.ServeHTTP(recorder, req2)
// body := recorder.Body.String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code ?

@aantono aantono force-pushed the Issue834 branch 5 times, most recently from 51a26e7 to 143dc17 Compare June 12, 2017 18:38
@aantono
Copy link
Contributor Author

aantono commented Jun 12, 2017

Rebased against master, removing changes on glide.yml, glide.lock and vendor. @ldez, this is all yours! ;)

@ldez ldez force-pushed the Issue834 branch 3 times, most recently from 4741f30 to c5d50d4 Compare June 12, 2017 21:52
@ldez
Copy link
Contributor

ldez commented Jun 12, 2017

@aantono it's ready. 🎉

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use safe.Go instead of the classic go routines.


report := time.NewTicker(pushInterval)

go datadogClient.SendLoop(report.C, "udp", address)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe.Go(func() {
	datadogClient.SendLoop(report.C, "udp", address)
})


report := time.NewTicker(pushInterval)

go statsdClient.SendLoop(report.C, "udp", address)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe.Go(func() {
	statsdClient.SendLoop(report.C, "udp", address)
})

@aantono
Copy link
Contributor Author

aantono commented Jul 18, 2017

No worries, You bet! Looks like the glide.lock is again in conflict... guess its time for @ldez to do his magic again! 😉

if datadogTicker == nil {
address := config.Address
if len(address) == 0 {
address = "localhost:8125"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we default to this address? Is this somehow conventional in Datadog?

Instinctively, I tend to go with requiring proper configuration and -- if missing -- return an error, unless a default makes sense.

Copy link
Contributor Author

@aantono aantono Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default DataDog statsD listener is running on localhost:8125, which is a well known port, so it is very common for people to not define it explicitly and use the default (be that in DataDog agent config, or in the clients) (See default dd-agent config for reference - https://github.com/DataDog/dd-agent/blob/master/datadog.conf.example#L163)

}
pushInterval, err := time.ParseDuration(config.PushInterval)
if err != nil {
log.Warnf("Unable to parse %s into pushInterval", config.PushInterval)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the log message should mention that we'll be falling back to a default value.


// InitDatadogClient initializes metrics pusher and creates a datadogClient if not created already
func InitDatadogClient(config *types.Datadog) *time.Ticker {
if datadogTicker == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we do not enter this function from multiple goroutines concurrently, and hence don't need to synchronize access?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. We only call InitDatadogClient once when the server starts.

Copy link
Contributor

@timoreimann timoreimann Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to comparing the ticker could then be to use sync.Once. This may also help us avoid accidentally initializing the client more than once if for whatever reason we toggle the ticker value inadvertently (say, due to a bug).

More of a suggestion though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sync.Once sounds like a good idea. I've tried to add it, but realized that I still need to keep track of the ticker reference, so that I could stop it once the shutdown occurs. Since I have to keep the ticker variable around anyways, do you want to introduce another variable to store the once, or just let it be and use the null check instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good enough in this case. Thanks for giving it a shot. 👍

}

// InitStatsdClient initializes metrics pusher and creates a statsdClient if not created already
func InitStatsdClient(config *types.Statsd) *time.Ticker {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire file, but especially this function look very similar to what is implemented in datadog.go. Is there any chance to factor out the duplicated code?

Copy link
Contributor

@ldez ldez Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's two different features, factorize don't seems a good idea.
For me, it's like consul.go, etcd.go or boltdb.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I thought about trying to generalize and factorize the code out, like @ldez mentioned, while the 2 files look very similar in pattern, they internally use different go-kit/metrics structures to initialize metrics, name them and return outbound structures, so there isn't much that can actually be shared, besides the logical steps of operation, which are similar due to the similarity in DataDog being an enhancement on top of StatsD protocol.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aantono The initial function is what I was mostly after. They look pretty much identical except for the ticker they reference and the receiver they trigger the loop on. That should be fairly easy to parameterize.

More importantly to me, refactoring should allow us to squash two fairly big tests into a single one.

When you say

besides the logical steps of operation, which are similar due to the similarity in DataDog being an enhancement on top of StatsD protocol.
(emphasis by me)

to me that's really a signal for how the two share certain attributes (i.e., the function in question) that we could reflect code-wise by a central implementation.

So that's my view. Maybe @emilevauge can step in and become the tipping point. I'll be happy to follow whatever majority constitutes. :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldez tbh, seeing three k/v store implementations spanning 30+ LOC each that really only differ by a single store type they pass into a function is rather an anti-example to me. 😮

Copy link
Contributor Author

@aantono aantono Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I looked once again at the InitDataDogClient and InitStatsDClient code and the only part they share is this:

address := config.Address
if len(address) == 0 {
    address = "localhost:8125"
}
pushInterval, err := time.ParseDuration(config.PushInterval)
if err != nil {
    log.Warnf("Unable to parse %s into pushInterval", config.PushInterval)
    pushInterval = 10 * time.Second
}

Given that it looks like boiler-plate code, it's really just a validation of config properties (but keep in mind that config is of different struct type: types.Datadog vs. types.Statsd), they just happen to both have address and pushInterval properties).
The rest of the function handles initializing different clients and each provider has their own ticker as well, which is a statically managed variable, so not that great for factorizing. IMHO it would be more code and will make it harder to read and reason about the code if we to try to come up with a base config type (not even sure what to call it) and then have the 2 concrete types encapsulate it. While I do agree that on the surface this does seem like a good possible case for boiler-plate reduction, once you look deeper, that does not turn out to be so, but curious to hear what @emilevauge thinks about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, you convinced me: I failed to see that the config structs are of different types as well. With that in mind, extracting anything doesn't seem too useful.

Appreciate the technical analysis to solve the matter. 👏

@@ -0,0 +1,52 @@
package middlewares
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look very similar to datadog_test.go too. Reuse opportunity?

Copy link
Contributor

@ldez ldez Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here, it's two different features.

server/server.go Outdated
}
if globalConfig.Web.Metrics.Datadog != nil {
metric := middlewares.NewDataDog(name)
log.Debugf("Configured DataDog Metrics pushing to %s once every %s", globalConfig.Web.Metrics.Datadog.Address, globalConfig.Web.Metrics.Datadog.PushInterval)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Metrics/metrics/

Copy link
Contributor Author

@aantono aantono Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to clarify, are you asking to replace "DataDog Metrics" part of the string in the log.Debugf with DataDog metrics lowercased metrics? If that's the case, I would argue that DataDog Metrics is a name-proper of the monitoring provider (just like Prometheus Metrics before it, but if you insist, would be happy to change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I think I was being too complicated: Yes, I just meant to use lower case in the log message.

I'm only familiar with Prometheus where I'm not aware of a proper name like Prometheus Metrics. If you could point me at any references (e.g., docs) showing this, I'll be happy to rest my case.

server/server.go Outdated
return metrics
if globalConfig.Web.Metrics.StatsD != nil {
metric := middlewares.NewStatsD(name)
log.Debugf("Configured StatsD Metrics pushing to %s once every %s", globalConfig.Web.Metrics.StatsD.Address, globalConfig.Web.Metrics.StatsD.PushInterval)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Metrics/metrics/

server/server.go Outdated
metricsEnabled := globalConfig.Web != nil && globalConfig.Web.Metrics != nil
if metricsEnabled {
if globalConfig.Web.Metrics.Datadog != nil {
middlewares.StopDatadogClient()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need all those ifs before? It looks like StopDatadogClient won't try to stop anything anyway unless the ticker was previously set (i.e., we have configured Datadog for sure).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't, at least not at the current moment, I just put them in there for security measure, in case later on the internals of StopDAtadogClient() change in a way that would require such a check to be done, but we could forget to do it. I would be happy to remove the if checks, if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that StopDatadogClient should decide if the client still needs to be stopped or not, rather than relying on static configuration parameters. The function already does this by means of using the ticker value as a flag, so overall I think we can remove the if blocks.

server/server.go Outdated
middlewares.StopDatadogClient()
}
if globalConfig.Web.Metrics.StatsD != nil {
middlewares.StopStatsdClient()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about the if'ing here.

@timoreimann
Copy link
Contributor

I think we're also missing a docs update somewhere to inform our users about the possibility to use Datadog or Statsd for metrics collection.

if statsdTicker == nil {
address := config.Address
if len(address) == 0 {
address = "localhost:8125"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this use port 9125 given what you said here about the default ports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, I confused StatsD with statsd-exporter project, which defaults to 9125. The default port for both StatsD and dogStatsD is indeed 8125, since the protocols are largely compatible (meaning you can send plain StatsD format data to dogStatsD agent, and it will accept it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Ready to merge once that dreaded merge conflict on the glide lock file (eek!) is resolved. :)

@aantono
Copy link
Contributor Author

aantono commented Jul 20, 2017

Sounds good... Looks like this is another opportunity for @ldez to shine and show off his mad "Glide" skills! 😜

Copy link
Contributor

@m3co-code m3co-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to comment on the closed PR. Didn't know a better place to raise my concerns. @aantono can you have a look at the comments and if necessary make a clean up PR?

Especially the point regarding DataDog/StatsD+Retries is concerning to me.

"github.com/go-kit/kit/metrics/dogstatsd"
)

var _ Metrics = (Metrics)(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this statement have any effect? I saw these kind of constructs to verify a concrete implementation is implementing an interface. To have the Metrics on the right hand side doesn't make any sense to me. Can you explain it or clean it up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right hand side should have been (*DataDog)(nil). Fixed.

return dd.reqDurationHistogram
}

func (dd *Datadog) getRetryCounter() metrics.Counter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As retryCounter never gets initialised won't this lead to a panic when having DataDog implementation (same for StatsD) + Retries activated? Did you try this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marco-jantke Great catch... the retry metric was added after the PR was already open, so I must've lost the change during one of the numerous rebases. The good news is that the code won't panic, since there is a m.retryMetrics != nil check before the counter is used, but the bad news is that we are missing retry metrics for DataDog & StatsD. Thanks a lot for noticing, I'll add those to the initialization block and open another PR.

return mm.retryCounter
}

func (mm *MultiMetrics) getWrappedMetrics() *[]Metrics {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS this method is used nowhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you are correct... Same PR that added retryMetrics, removed the getWrappedMetrics() method. It was originally present on the initial Metrics interface. Will clean-up

// and the number of requests partitioned by status code and method.
// - number of requests partitioned by status code and method
// - request durations
// - amount of retries happened
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not expose amount of retries happened AFAICS. Same for DataDog implementation.

m3co-code added a commit to m3co-code/traefik that referenced this pull request Aug 22, 2017
This merge removes the integration of DataDog and StatsD that was
introduced with this PR: traefik#1701

This integration is highly incompatible with our current Prometheus
metrics implementation that we have in dustify. Therefore everything
specific to those metrics integration was removed, except the glide
adaption. They don't hurt for now and better not to start getting merge
conflicts in them in there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants