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 for pluggable metric reporting #81

Merged
merged 4 commits into from
Oct 2, 2024
Merged

Support for pluggable metric reporting #81

merged 4 commits into from
Oct 2, 2024

Conversation

leviramsey
Copy link
Contributor

References #80

This doesn't do the Cinnamon bit (how that bit would work is a mystery to me), but at least this would allow users to plug in the official CloudWatch MetricsPublisher or manually setup Cinnamon metrics.

Roughly used the Akka Circuit Breaker instrumentation as a guide.

Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

Should we have a built-in provider for the CloudWatch metrics publisher?

@pvlugter
Copy link
Contributor

pvlugter commented Oct 1, 2024

And would be useful to document this.

@leviramsey
Copy link
Contributor Author

Should we have a built-in provider for the CloudWatch metrics publisher?

Not sure... if we were to have support for full config (since it looks to have a fairly full-featured builder), then yes; otherwise (like if it's just the defaults), then I don't know how useful it would actually be.

Will write up some docs tonight.

@leviramsey
Copy link
Contributor Author

NB: I realized that metricProviderFor was a terrible name... metricPublisherFor better captures what's actually being provided.

(Of course, I suppose the interface should be MetricPublisherProvider, but something about that name just doesn't feel right)

@leviramsey
Copy link
Contributor Author

I almost think a CloudWatch provider (and even this interface, since it's not DynamoDB specific) should be part of a general AWS support extension. But for the time being, this is "enough".

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

looking good

@leviramsey leviramsey merged commit f62012d into main Oct 2, 2024
3 checks passed
@leviramsey leviramsey deleted the metrics branch October 2, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants