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

[metrics/cloudwatch] Feature request: ability to publish a sample of metrics for histograms #980

Open
jszwedko opened this issue May 7, 2020 · 5 comments

Comments

@jszwedko
Copy link

jszwedko commented May 7, 2020

The cloudwatch metrics exporter currently has support for publishing histograms as a set of metrics of the histogram quantiles (e.g. p50, p90, p95, p99).

What I would like to do, is have the ability to instead publish a random sample of the recorded histogram values and let CloudWatch do the bucketing. This is because I have multiple instances that I would like to publish the same metric for without labeling them with a label unique to the instance. If there are less than 150 observations (the max for an individual PutMetricsData request to AWS), they would all be sent.

I pushed some changes up to demonstrate what I mean: https://github.com/go-kit/kit/compare/master...jszwedko:metrics/cloudwatch/add-ability-to-publish-histogram-values?expand=1 . If you all are amenable to this change, I can continue implementing it and open a PR.

Thanks!

@peterbourgon
Copy link
Member

I think this is really pushing the limits on what the Go kit metrics package can or should provide to consumers. It was only ever meant to be a very thin glue layer, a set of minimal adapters, so that the metrics interfaces could abstract over different concrete providers. It's not intended to be a place to implement value-add features like this.

@jszwedko
Copy link
Author

jszwedko commented May 8, 2020

Thanks for the response!

I do realize that it is adding a feature to the cloudwatch exporter, but I'm having a hard time understanding the distinction between the feature I'd like to add, and say, the exporter's current support for configuring histogram percentiles.

Do you mean to say that this is better done by implementing a new exporter in my repository? I could definitely do that. It would be very nice to take advantage of the internal lv package in this go-kit/metrics though. Would you consider exporting that package so that external metrics exporters could use it? Either within go-kit or as a standalone repo.

A trade-off of implementing my own exporter that the go-kit metrics exporter "ecosystem" becomes more fragmented. I did like that I could just drop in the cloudwatch exporter in go-kit which is, presumably, already battletested.

@peterbourgon
Copy link
Member

I'm having a hard time understanding the distinction between the feature I'd like to add, and say, the exporter's current support for configuring histogram percentiles.

It was probably a mistake to accept that "value add" stuff to Go kit.

Do you mean to say that this is better done by implementing a new exporter in my repository?

That might be an approach. My primary motivation is avoiding accepting the maintenance burden for features that don't necessarily need to be in the Go kit repository.

A trade-off of implementing my own exporter that the go-kit metrics exporter "ecosystem" becomes more fragmented.

It was never the intent that Go kit's package metrics should represent or define any kind of ecosystem. It was only the intent that it should provide the thinnest possible adapter layer, to allow "brown field" migrations from one metrics backend to another.

@peterbourgon
Copy link
Member

It would be very nice to take advantage of the internal lv package in this go-kit/metrics though. Would you consider exporting that package so that external metrics exporters could use it? Either within go-kit or as a standalone repo.

With the caveat that I don't consider that package particularly good anymore: yes, I can do that. Do you have opinions about where it should live? All else equal I would simply put it somewhere in the main repo, not underneath an internal subdirectory. Would that work?

@jszwedko
Copy link
Author

jszwedko commented May 8, 2020

It was probably a mistake to accept that "value add" stuff to Go kit.

It was never the intent that Go kit's package metrics should represent or define any kind of ecosystem. It was only the intent that it should provide the thinnest possible adapter layer, to allow "brown field" migrations from one metrics backend to another.

Gotcha, yeah, I think I understand better the distinction you are making for the purpose of go-kit. It seems like the exporters, in general, don't really fit in here, if I'm following correctly.

With the caveat that I don't consider that package particularly good anymore: yes, I can do that. Do you have opinions about where it should live? All else equal I would simply put it somewhere in the main repo, not underneath an internal subdirectory. Would that work?

Now I'm curious what you don't like about it 😄 but yes, somewhere in the main repo, not under internal would suffice for my purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants