Configure appsignals processor on EKS with cluster name #980

bjrara · 2023-12-04T23:23:20Z

Description of the issue

Currently, the awsappsignals processor reads cluster name from resource attributes which is extracted from EC2 tag kubernetes.io/cluster/my-cluster by resourcedetection processor. Most of time, it's working because EKS adds the tag to nodes during provisioning. However, according to Amazon EKS VPC and subnet requirements and considerations, this tag is not a hard dependency for EKS, thus if the customer removes the tag accidentally (or on purpose), our metrics will fail to be generated with correct EKS cluster name dimension, and cause false alarming or missin graphs if the metrics are used in dashboards or alarms/SLOs.

Description of changes

Make the cluster name as a mandatory field to bootstrap awsappsignals processor on EKS;
Change the field resolvers from string array to object array;
Refactor the package layout of awsappsignals;

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Unit tests.
Manual E2E tests on EKS and EC2.

Requirements

Before commit the code, please do the following steps.

Run make fmt and make fmt-sh
Run make lint

bjrara · 2023-12-04T23:23:33Z

@mxiamxia @pxaws

mxiamxia

LGTM! thanks!

translator/translate/otel/processor/awsappsignals/translator.go

plugins/processors/awsappsignals/config/resolvers.go

bjrara · 2023-12-08T19:08:02Z

@lisguo @sky333999 Can you help review?

plugins/processors/awsappsignals/config/resolvers_test.go

translator/translate/otel/processor/awsappsignals/translator.go

codecov-commenter · 2023-12-14T21:41:06Z

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (96d4763) 57.58% compared to head (e870ec6) 63.02%.
Report is 470 commits behind head on main.

Files	Patch %	Lines
cfg/aws/credentials.go	0.00%	22 Missing ⚠️
cfg/aws/shared_config.go	62.50%	15 Missing ⚠️
cfg/envconfig/envconfig.go	50.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
+ Coverage   57.58%   63.02%   +5.43%     
==========================================
  Files         370      360      -10     
  Lines       17548    18202     +654     
==========================================
+ Hits        10105    11471    +1366     
+ Misses       6848     6142     -706     
+ Partials      595      589       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

plugins/processors/awsappsignals/config/resolvers.go

translator/translate/otel/processor/awsappsignals/translator.go

sky333999 · 2023-12-18T21:39:33Z

plugins/processors/awsappsignals/internal/resolver/attributesresolver.go

-func newHostedInAttributeResolver(attributeMap map[string]string) *hostedInAttributeResolver {
+func newHostedInAttributeResolver(name string, attributeMap map[string]string) *hostedInAttributeResolver {
+	if name == "" {
+		name = "Generic"


Could we instead use the PlatformGeneric const here as well instead of a string or do we want this to intentionally be Generic and not generic?

We use Generic following the examples provided by CloudWatch metrics: https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_concepts.html.

Added const PlatformGeneric.

translator/translate/otel/processor/awsappsignals/translator.go

lisguo · 2023-12-19T19:03:27Z

plugins/processors/awsappsignals/factory.go

-		// TODO: change default config when other resolvers are supported
-		Resolvers: []string{"eks"},
+	return &appsignalsconfig.Config{
+		Resolvers: []appsignalsconfig.Resolver{},


is there a use case when we have a default resolver? What happens then?

I would never expect the default resolver to be initied for real use case. In case it happens, it will fail the config validation with unknown resolver error.

lisguo · 2023-12-19T19:11:32Z

translator/translate/otel/processor/awsappsignals/translator.go

 	if common.IsAppSignalsKubernetes() {
-		cfg.Resolvers = []string{"eks"}
+		if !hostedInConfigured {
+			hostedIn = util.GetClusterNameFromEc2Tagger()


nice, I didn't even know we had this function

lisguo · 2023-12-20T15:19:50Z

Seems like there are some unit test failures

 Error: plugins/processors/awsappsignals/config/config_test.go:14:49: not enough arguments in call to NewGenericResolver
	have ()
	want (string)

plugins/processors/awsappsignals/config/config_test.go

bjrara · 2023-12-20T19:17:14Z

The test failed for some irrelevant issue

2023/12/20 18:18:06 E! [cloudwatchlogs.test] Stop requested after 0 retries to G/S failed for PutLogEvents, request dropped.
--- FAIL: TestLongMessageGetsTruncated (0.02s)
    pusher_test.go:187: PutLogEvents called with incorrect number of message, expecting 1, but 0 received
FAIL
coverage: 39.1% of statements
FAIL	github.com/aws/amazon-cloudwatch-agent/plugins/outputs/cloudwatchlogs	3.131s

bjrara requested a review from a team as a code owner December 4, 2023 23:23

mxiamxia previously approved these changes Dec 4, 2023

View reviewed changes

bjrara dismissed mxiamxia’s stale review via 9038bf2 December 5, 2023 02:43

bjrara force-pushed the main branch 2 times, most recently from 9038bf2 to 532305d Compare December 5, 2023 03:04

mxiamxia previously approved these changes Dec 5, 2023

View reviewed changes

bjrara dismissed mxiamxia’s stale review via 1ad1e3f December 5, 2023 18:21

bjrara force-pushed the main branch from 532305d to 1ad1e3f Compare December 5, 2023 18:21

pxaws reviewed Dec 6, 2023

View reviewed changes

translator/translate/otel/processor/awsappsignals/translator.go Outdated Show resolved Hide resolved

plugins/processors/awsappsignals/config/resolvers.go Show resolved Hide resolved

bjrara force-pushed the main branch from 1ad1e3f to 2476ee7 Compare December 6, 2023 20:59

pxaws approved these changes Dec 7, 2023

View reviewed changes

bjrara force-pushed the main branch from 2476ee7 to 23c0fab Compare December 8, 2023 19:06

JayPolanco reviewed Dec 11, 2023

View reviewed changes

plugins/processors/awsappsignals/config/resolvers_test.go Show resolved Hide resolved

mxiamxia previously approved these changes Dec 11, 2023

View reviewed changes

JayPolanco reviewed Dec 11, 2023

View reviewed changes

translator/translate/otel/processor/awsappsignals/translator.go Outdated Show resolved Hide resolved

bjrara dismissed mxiamxia’s stale review via 09a9572 December 11, 2023 19:23

bjrara force-pushed the main branch from 23c0fab to 09a9572 Compare December 11, 2023 19:23

pass cluster name down to appsignals processor

ff3a19b

bjrara force-pushed the main branch from 09a9572 to ff3a19b Compare December 14, 2023 19:31

JayPolanco previously approved these changes Dec 14, 2023

View reviewed changes

sky333999 reviewed Dec 15, 2023

View reviewed changes

plugins/processors/awsappsignals/config/resolvers.go Outdated Show resolved Hide resolved

translator/translate/otel/processor/awsappsignals/translator.go Outdated Show resolved Hide resolved

bjrara dismissed JayPolanco’s stale review via 57c45b4 December 15, 2023 23:45

bjrara force-pushed the main branch from 57c45b4 to 1f931f8 Compare December 16, 2023 01:03

sky333999 reviewed Dec 18, 2023

View reviewed changes

bjrara force-pushed the main branch from 1f931f8 to cbc237b Compare December 18, 2023 22:55

sky333999 previously approved these changes Dec 18, 2023

View reviewed changes

bjrara dismissed sky333999’s stale review via b121450 December 18, 2023 23:05

bjrara force-pushed the main branch 2 times, most recently from b121450 to 583ff41 Compare December 18, 2023 23:08

sky333999 previously approved these changes Dec 18, 2023

View reviewed changes

lisguo previously approved these changes Dec 19, 2023

View reviewed changes

bjrara dismissed stale reviews from lisguo and sky333999 via 9c8e255 December 19, 2023 20:36

bjrara force-pushed the main branch 2 times, most recently from 9c8e255 to f424a1b Compare December 19, 2023 20:39

JayPolanco reviewed Dec 20, 2023

View reviewed changes

plugins/processors/awsappsignals/config/config_test.go Outdated Show resolved Hide resolved

introduce hosted_in field into app_signals

e870ec6

bjrara force-pushed the main branch from f424a1b to e870ec6 Compare December 20, 2023 18:11

JayPolanco approved these changes Dec 20, 2023

View reviewed changes

lisguo approved these changes Dec 21, 2023

View reviewed changes

JayPolanco merged commit f2e1251 into aws:main Dec 21, 2023
6 checks passed

harrryr pushed a commit that referenced this pull request Jan 2, 2024

Configure appsignals processor on EKS with cluster name (#980)

b7d9016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure appsignals processor on EKS with cluster name #980

Configure appsignals processor on EKS with cluster name #980

bjrara commented Dec 4, 2023 •

edited

Loading

bjrara commented Dec 4, 2023

mxiamxia left a comment

bjrara commented Dec 8, 2023

codecov-commenter commented Dec 14, 2023 •

edited

Loading

sky333999 Dec 18, 2023

bjrara Dec 18, 2023

bjrara Dec 18, 2023

lisguo Dec 19, 2023

bjrara Dec 19, 2023

lisguo Dec 19, 2023

lisguo commented Dec 20, 2023

bjrara commented Dec 20, 2023

Configure appsignals processor on EKS with cluster name #980

Configure appsignals processor on EKS with cluster name #980

Conversation

bjrara commented Dec 4, 2023 • edited Loading

Description of the issue

Description of changes

License

Tests

Requirements

bjrara commented Dec 4, 2023

mxiamxia left a comment

Choose a reason for hiding this comment

bjrara commented Dec 8, 2023

codecov-commenter commented Dec 14, 2023 • edited Loading

Codecov Report

sky333999 Dec 18, 2023

Choose a reason for hiding this comment

bjrara Dec 18, 2023

Choose a reason for hiding this comment

bjrara Dec 18, 2023

Choose a reason for hiding this comment

lisguo Dec 19, 2023

Choose a reason for hiding this comment

bjrara Dec 19, 2023

Choose a reason for hiding this comment

lisguo Dec 19, 2023

Choose a reason for hiding this comment

lisguo commented Dec 20, 2023

bjrara commented Dec 20, 2023

bjrara commented Dec 4, 2023 •

edited

Loading

codecov-commenter commented Dec 14, 2023 •

edited

Loading