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

Expose AWS Metrics as an array in update.go #31

Merged
merged 3 commits into from
Apr 8, 2022

Conversation

kgeckhart
Copy link

@kgeckhart kgeckhart commented Apr 8, 2022

This should hopefully make it a lot harder to skip registering a metric, which I already did.

Also deleted the release.yml since it attempts to publish to the upstream repo, 😬
Edit: and adjusted lint + build to run on our live branch

Copy link

@matthewnolf matthewnolf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Name: "yace_cloudwatch_dmsapi_requests_total",
Help: "Help is not implemented yet.",
})
)

Choose a reason for hiding this comment

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

Should Metrics slice be declared next to the metrics now

Copy link
Author

Choose a reason for hiding this comment

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

My thought is that update.go is the library entrypoint so if we centralize important pieces of the library API it can make it more discoverable/obvious. We might be the only people using it as a library but might help upstream maintenance as a reminder there's multiple entrypoints. Writing this made think it might be a good idea to add this info to the upstream README. WDYT?

Choose a reason for hiding this comment

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

We might be the only people using it as a library but might help upstream maintenance as a reminder there's multiple entrypoints. Writing this made think it might be a good idea to add this info to the upstream README. WDYT?

+1

Copy link

@ferruvich ferruvich left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kgeckhart kgeckhart merged commit 69f6286 into live Apr 8, 2022
@kgeckhart kgeckhart deleted the keckhart/export-metric-array branch April 8, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants