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

Merge 0.34.0 and 0.33.0 #30

Merged
merged 2 commits into from
Apr 6, 2022
Merged

Merge 0.34.0 and 0.33.0 #30

merged 2 commits into from
Apr 6, 2022

Conversation

kgeckhart
Copy link

Merges in the latest version of YACE mostly to pickup changes related to propagating context through the AWS calls. There's other changes which shouldn't really impact us.

Full changes from upstream here, nerdswords/yet-another-cloudwatch-exporter@v0.32.0-alpha...v0.34.0-alpha. This was a pain because I never pushed the logging changes upstream which I need to do.

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.

🚀

# 0.34.0-alpha
* Update dependencies
* Add weekly dependabot updates (jylitalo)
* Add support for regional sts endpoints (matt-mercer)

Choose a reason for hiding this comment

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

👀
@cristiangreco I don't remember exactly what was the problem we had trying regional STS endpoints, but the blocker was before hitting the exporter, right?

Choose a reason for hiding this comment

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

I think our problem was around "job validation". Anyway chatted with Kyle yesterday about this specific change and it shouldn't impact us as we use a custom session.

Choose a reason for hiding this comment

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

Thanks! 🙌

cmd/yace/main.go Outdated
@@ -207,15 +215,15 @@ func (s *scraper) scrape(ctx context.Context, cache exporter.SessionCache) {
defer sem.Release(1)

newRegistry := prometheus.NewRegistry()
for _, counter := range []prometheus.Counter{exporter.CloudwatchAPICounter, exporter.CloudwatchAPIErrorCounter, exporter.CloudwatchGetMetricDataAPICounter, exporter.CloudwatchGetMetricStatisticsAPICounter, exporter.ResourceGroupTaggingAPICounter, exporter.AutoScalingAPICounter, exporter.ApiGatewayAPICounter, exporter.TargetGroupsAPICounter} {
for _, counter := range []prometheus.Counter{exporter.CloudwatchAPICounter, exporter.CloudwatchAPIErrorCounter, exporter.CloudwatchGetMetricDataAPICounter, exporter.CloudwatchGetMetricStatisticsAPICounter, exporter.ResourceGroupTaggingAPICounter, exporter.AutoScalingAPICounter, exporter.ApiGatewayAPICounter, exporter.TargetGroupsAPICounter, exporter.DmsAPICounter} {

Choose a reason for hiding this comment

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

I think we're missing exporter.Ec2APICounter here (should be 10 in total).

Copy link
Author

Choose a reason for hiding this comment

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

Great catch! Perhaps I should do a follow up to export an array which contains all of them instead to help avoid this.

Copy link

@cristiangreco cristiangreco left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment about a counter.

@kgeckhart kgeckhart merged commit 6ca630f into live Apr 6, 2022
@kgeckhart kgeckhart deleted the keckhart/merge-0.34.0-alpha branch April 6, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants