Update cloudwatch exporter dependency #839

andriikushch · 2024-05-14T09:43:29Z

PR Description

This PR adds a new configuration parameter for the Cloudwatch exporter. It allows to specify the AWS SDK version to use. v1 is the default version.

It was achieved by updating and integrating the version of the github.com/nerdswords/yet-another-cloudwatch-exporter package and all dependencies that it depends on.

According to the https://github.com/nerdswords/yet-another-cloudwatch-exporter/blob/a275f94e5d33b3b5330ecfced86c290e1db05928/pkg/config/feature_flags.go#L10 , using the AWS SDK v2, should improve performance.

Breaking change:

After this change, using aliases for the AWS service names is no longer allowed.

Which issue(s) this PR fixes

N.A.

A related change in the Agent project:

Update cloudwatch exporter dependency agent#6835

Notes to the Reviewer

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

…porter-dependency- # Conflicts: # CHANGELOG.md

clayton-cornell · 2024-05-16T22:15:08Z

Doc changes look OK.

Should this be backported? Or only in next?

…porter-dependency- # Conflicts: # go.mod # go.sum

andriikushch · 2024-05-21T08:29:26Z

Doc changes look OK.

Should this be backported? Or only in next?

@clayton-cornell It would be nice to backport it. The same change is in the Agent repository:

Update cloudwatch exporter dependency agent#6835.

clayton-cornell · 2024-05-22T16:21:19Z

Added backports to Alloy v1.0 and v1.1

There's a parallel Agent PR already in process, so added the label backport-to-agent:no

kgeckhart · 2024-05-22T18:02:33Z

internal/component/prometheus/exporter/cloudwatch/config.go

@@ -37,6 +40,7 @@ type Arguments struct {
 	Discovery             []DiscoveryJob        `alloy:"discovery,block,optional"`
 	Static                []StaticJob           `alloy:"static,block,optional"`
 	DecoupledScrape       DecoupledScrapeConfig `alloy:"decoupled_scraping,block,optional"`
+	AWSSDKVersion         string                `alloy:"aws_sdk_version,attr,optional"`


Given there's only two options v1 and v2 with v1 being deprecated with an EOL date I think this should really be a boolean to enable or disable sdk v2 vs a string like this. We aren't anticipating the need to support a v3 anytime soon.

CHANGELOG.md

kgeckhart · 2024-05-22T19:39:26Z

internal/component/prometheus/exporter/cloudwatch/config.go

@@ -26,6 +28,7 @@ var defaults = Arguments{
 		Enabled:        false,
 		ScrapeInterval: 5 * time.Minute,
 	},
+	AWSSDKVersion: "v2",


I think we should default to v1 for now to prevent breaking anyone on upgrade. We can make v2 the default following YACE changing.

Let's do it 👍

Co-authored-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>

kgeckhart

Did you have a chance to run it locally as well?

docs/sources/reference/components/prometheus.exporter.cloudwatch.md

…ch.md Co-authored-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>

andriikushch · 2024-05-28T11:55:39Z

Did you have a chance to run it locally as well?

Yes. I created the config with 3 cloudwatch exporters: without the new parameter, with aws_sdk_version_v2 set to true and to false. I used it to get EC2 metrics. And compared aws_ec2_cpuutilization_average graphs. They look similar (I think the difference is depending on the exact time of the getting data):

…porter-dependency- # Conflicts: # go.mod # go.sum

kgeckhart · 2024-05-30T10:37:20Z

internal/static/integrations/cloudwatch_exporter/docs/doc.go

@@ -61,7 +61,7 @@ func generateServicesDocSection() string {
 	var sb strings.Builder
 	for _, supportedSvc := range yaceConf.SupportedServices {
 		sb.WriteString(
-			fmt.Sprintf("- Namespace: `%s` or Alias: `%s`\n", supportedSvc.Namespace, supportedSvc.Alias),


You will need to run this to update the service list in the docs. ATM they all still have the Alias.

tpaschalis · 2024-06-03T10:10:57Z

.github/workflows/test.yml

@@ -23,4 +23,4 @@ jobs:
        go-version: "1.22"
        cache: true
    - name: Test
-      run: make GO_TAGS="nodocker" test
+      run: CGO_LDFLAGS="-ld_classic $CGO_LDFLAGS" make GO_TAGS="nodocker" test


Why is this change needed?

@tpaschalis, without CGO_LDFLAGS="-ld_classic build fails on mac.

tpaschalis

Sorry to block this, doing it to avoid getting this accidentally merged (I've yet to review the entire PR).

As per our backwards compatibility guarantees, we're no longer doing breaking changes in GA functionality.

Instead we'll need to find a way to support both the previous and current type values without affecting users' config files and queue any breaking changes for a v2.0 release in the future with the v2.0-breaking-change label

kgeckhart · 2024-06-03T14:08:53Z

Instead we'll need to find a way to support both the previous and current type values without affecting users' config files and queue any breaking changes for a v2.0 release in the future with the v2.0-breaking-change label

I think this particular change is covered under this exception to the backwards compatibility guarantee,

Upstream changes: Much of the functionality of {{< param "PRODUCT_NAME" >}} is built on top of other software, such as OpenTelemetry Collector and Prometheus. If upstream software breaks compatibility, we may need to reflect this in {{< param "PRODUCT_NAME" >}}.

As the change is being driven by the upstream exporter in this case.

tpaschalis · 2024-06-03T14:25:10Z

But also,

We try, whenever possible, to resolve these issues without breaking compatibility.

It's always a tradeoff, but I think it's a good opportunity to try and fix this without affecting users. On a surface level it doesn't feel like a compatibility layer to support aliasing these types wouldn't be so intrusive. WDYT?

kgeckhart · 2024-06-03T14:41:11Z

But also,

We try, whenever possible, to resolve these issues without breaking compatibility.

It's always a tradeoff, but I think it's a good opportunity to try and fix this without affecting users. On a surface level it doesn't feel like a compatibility layer to support aliasing these types wouldn't be so intrusive. WDYT?

I agree in principal, but @cristiangreco intentionally dropped these aliases upstream which is why I'm in favor of dropping them here as well. They do not have any AWS specific meaning to them that makes them valuable for a customer and how they were used was harmful nerdswords/yet-another-cloudwatch-exporter#1131.

cristiangreco · 2024-06-05T07:55:40Z

But also,

We try, whenever possible, to resolve these issues without breaking compatibility.

It's always a tradeoff, but I think it's a good opportunity to try and fix this without affecting users. On a surface level it doesn't feel like a compatibility layer to support aliasing these types wouldn't be so intrusive. WDYT?

I agree in principal, but @cristiangreco intentionally dropped these aliases upstream which is why I'm in favor of dropping them here as well. They do not have any AWS specific meaning to them that makes them valuable for a customer and how they were used was harmful nerdswords/yet-another-cloudwatch-exporter#1131.

Yes the upstream change was intentional, so in theory the usage of aliases should be dropped in alloy as well.

andriikushch · 2024-06-07T13:16:13Z

Hi @tpaschalis,

@kgeckhart and I discussed some options for this case. We agreed that we could introduce a translation layer that will also produce warning logs when aliases are used and inform that support of the aliases will be dropped in the upcoming releases.

This will give users enough time to update their configs and allow them to benefit from the new component version immediately.

What do you think?

tpaschalis · 2024-06-07T13:31:39Z

I don't see the benefit of removing the translation layer though. Do you anticipate there being it an effort to maintain?

On surface it sounds like a simple string-to-string mapping that helps maintain user confidence in our backwards compatibility guarantees.

In any case, other members of the @grafana/grafana-alloy-maintainers can chime in here with their opinion, let's see what they say

mattdurham · 2024-06-07T13:44:20Z

If the aliases are indeed considered a bug then I feel that's reasonable to not have them and would follow our backwards compatibility. Otherwise I would strongly push for the aliasing.

kgeckhart · 2024-06-07T14:54:08Z

The implementation is not going to be hard to do it's more the extra layer of indirection we have to consider when attempting to understand a customers configuration. The alias is not a real value to AWS while what we are forcing someone to use instead is the AWS CloudWatch namespace. Some aliases are easily relatable back to the CloudWatch namespace but for things like gwlb I would have to look at our mapping layer or google it to recognize it as AWS/GatewayELB.

andriikushch added 9 commits May 14, 2024 11:41

Update cloudwatch exporter dependency

023c06c

Fix imports

fb934e7

Fix converter tests

09f3380

Add documentation

6df9bdc

Change CHANGELOG.md

0802a26

Revert some changes in the test

b2d50c6

Merge branch 'refs/heads/main' into andriikushch/update-cloudwatch-ex…

4a109b3

…porter-dependency- # Conflicts: # CHANGELOG.md

Update CHANGELOG.md

c743403

Fix tests

a9a9f60

andriikushch marked this pull request as ready for review May 16, 2024 14:29

andriikushch requested a review from clayton-cornell as a code owner May 16, 2024 14:29

andriikushch requested a review from kgeckhart May 16, 2024 14:29

andriikushch mentioned this pull request May 16, 2024

Update cloudwatch exporter dependency grafana/agent#6835

Open

4 tasks

andriikushch added outdated-dependency Related to an outdated dependency. dependencies Pull requests that update a dependency file labels May 16, 2024

clayton-cornell requested a review from a team May 16, 2024 22:14

clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label May 16, 2024

andriikushch added 2 commits May 21, 2024 10:06

Merge branch 'refs/heads/main' into andriikushch/update-cloudwatch-ex…

6566866

…porter-dependency- # Conflicts: # go.mod # go.sum

Update go.mod

dc04b58

Add -ld_classic to the CGO_LDFLAGS

6d6a0f6

clayton-cornell added backport release/v1.0 backport-to-agent:no PR should NOT be backported to the agent repo. backport release/v1.1 labels May 22, 2024

kgeckhart requested a review from thepalbi May 22, 2024 17:58

kgeckhart reviewed May 22, 2024

View reviewed changes

andriikushch removed backport release/v1.0 backport release/v1.1 labels May 23, 2024

Update CHANGELOG.md

06893a6

Co-authored-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>

andriikushch added backport release/v1.0 backport release/v1.1 labels May 23, 2024

andriikushch added 3 commits May 23, 2024 10:04

Implements comments from the PR

947b23b

Rename variable

85e9bf3

Rename variable

a426e78

andriikushch requested a review from kgeckhart May 23, 2024 08:29

kgeckhart reviewed May 23, 2024

View reviewed changes

docs/sources/reference/components/prometheus.exporter.cloudwatch.md Outdated Show resolved Hide resolved

Update docs/sources/reference/components/prometheus.exporter.cloudwat…

0ddfaa2

…ch.md Co-authored-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>

andriikushch requested a review from kgeckhart May 28, 2024 11:55

andriikushch added 2 commits May 29, 2024 11:04

Merge branch 'refs/heads/main' into andriikushch/update-cloudwatch-ex…

39c4fe9

…porter-dependency- # Conflicts: # go.mod # go.sum

Update go.mod

46be868

kgeckhart approved these changes May 30, 2024

View reviewed changes

Update prometheus.exporter.cloudwatch.md

9851b72

tpaschalis reviewed Jun 3, 2024

View reviewed changes

tpaschalis requested changes Jun 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cloudwatch exporter dependency #839

Update cloudwatch exporter dependency #839

andriikushch commented May 14, 2024 •

edited

clayton-cornell commented May 16, 2024

andriikushch commented May 21, 2024

clayton-cornell commented May 22, 2024

kgeckhart May 22, 2024

andriikushch May 23, 2024

kgeckhart May 22, 2024

andriikushch May 23, 2024

kgeckhart left a comment

andriikushch commented May 28, 2024

kgeckhart May 30, 2024

tpaschalis Jun 3, 2024

andriikushch Jun 3, 2024

tpaschalis left a comment

kgeckhart commented Jun 3, 2024

tpaschalis commented Jun 3, 2024

kgeckhart commented Jun 3, 2024 •

edited

cristiangreco commented Jun 5, 2024

andriikushch commented Jun 7, 2024

tpaschalis commented Jun 7, 2024

mattdurham commented Jun 7, 2024

kgeckhart commented Jun 7, 2024 •

edited

Update cloudwatch exporter dependency #839

Are you sure you want to change the base?

Update cloudwatch exporter dependency #839

Conversation

andriikushch commented May 14, 2024 • edited

PR Description

Breaking change:

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

clayton-cornell commented May 16, 2024

andriikushch commented May 21, 2024

clayton-cornell commented May 22, 2024

kgeckhart May 22, 2024

Choose a reason for hiding this comment

andriikushch May 23, 2024

Choose a reason for hiding this comment

kgeckhart May 22, 2024

Choose a reason for hiding this comment

andriikushch May 23, 2024

Choose a reason for hiding this comment

kgeckhart left a comment

Choose a reason for hiding this comment

andriikushch commented May 28, 2024

kgeckhart May 30, 2024

Choose a reason for hiding this comment

tpaschalis Jun 3, 2024

Choose a reason for hiding this comment

andriikushch Jun 3, 2024

Choose a reason for hiding this comment

tpaschalis left a comment

Choose a reason for hiding this comment

kgeckhart commented Jun 3, 2024

tpaschalis commented Jun 3, 2024

kgeckhart commented Jun 3, 2024 • edited

cristiangreco commented Jun 5, 2024

andriikushch commented Jun 7, 2024

tpaschalis commented Jun 7, 2024

mattdurham commented Jun 7, 2024

kgeckhart commented Jun 7, 2024 • edited

andriikushch commented May 14, 2024 •

edited

kgeckhart commented Jun 3, 2024 •

edited

kgeckhart commented Jun 7, 2024 •

edited