Adding Statuscode Handler to Agent Health Extension #1423

Paramadon · 2024-11-12T20:47:17Z

Description of the Issue

The CloudWatch Agent lacked an agenthealth configuration to monitor API health by tracking HTTP status codes for specific responses. Monitoring the status codes (200, 400, 408, 419, and 429) across all APIs is critical for diagnosing issues and ensuring comprehensive observability of API behaviors. Without this configuration, users were unable to quickly identify trends in API health metrics or correlate specific status codes with performance issues.

Changes Made

Added agenthealth Configuration:
- Configured the CloudWatch Agent to track the following status codes across all APIs:
  - 200: Success
  - 400: Bad Request
  - 408: Request Timeout
  - 419: Authentication Timeout
  - 429: Too Many Requests
- This ensures that health metrics for these common response codes are captured and reported.
Updated CloudWatch Agent Configuration JSON:
- Introduced a new section under the metrics collection configuration to specify agenthealth metrics.
- Ensured compatibility with existing metrics and logs collection by integrating the new configuration seamlessly.
Validated the Configuration:
- Tested the updated configuration with the CloudWatch Agent to ensure the new metrics are collected and reported correctly.
- Confirmed that the metrics appear as expected in CloudWatch for all specified APIs.

Impact

Provides real-time visibility into API status codes, helping identify anomalies or patterns that indicate issues.
Enhances observability for critical services by adding detailed health metrics.

Testing

Deployed the updated CloudWatch Agent health configuraion
Verified that metrics for status codes 200, 400, 408, 419, and 429 are captured in the header as you can see from the image below:

…s from status codes

…andler

mitali-salvi

Could you fix the linting issues ?

mitali-salvi · 2024-11-21T16:59:59Z

extension/agenthealth/config.go

@@ -12,6 +12,7 @@ import (
 type Config struct {
 	IsUsageDataEnabled bool              `mapstructure:"is_usage_data_enabled"`
 	Stats              agent.StatsConfig `mapstructure:"stats"`
+	StatusCodeOnly     *bool             `mapstructure:"is_status_code_only,omitempty"`


nitpick - can be renamed to is_only_status_code_enabled
also keep it consistent with IsUsageDataEnabled with IsOnlyStatusCodeEnabled

Also any reason to make it a pointer ?

A bool with omitempty in the tag is enough.

Agreed with mitali. Instead of a pointer, can we treat the default value as false?

Additionally, can you explain a bit more what this field is used for? It seems like a way to set the agent health extension such that it turns the other things off. I think that may be confusing because you could set IsUsageDataEnabled to true and also set StatusCodeOnly to true, so it's not clear what it would actually do (without looking at the implementation).

That makes sense. I will rename it. The pointer is used so we can emit it even if it's empty (nil). As for its functionality, I can add a comment, but the main purpose is to ensure that we only use the statuscodehandler when calling the agenthealthextension for a processor, exporter, or receiver, without including other handlers like processstats, clientstats, etc.

extension/agenthealth/handler/stats/agent/agent.go

extension/agenthealth/handler/stats/provider/process_test.go

extension/agenthealth/handler/stats/provider/interval.go

tool/clean/clean_host/clean_host.go

extension/agenthealth/handler/stats/agent/agent_test.go

extension/agenthealth/handler/stats/agent/agent.go

extension/agenthealth/handler/stats/agent/agent_test.go

extension/agenthealth/handler/stats/handler.go

extension/agenthealth/handler/stats/provider/statuscode.go

internal/ecsservicediscovery/containerinstanceprocessor.go

extension/agenthealth/handler/stats/provider/statuscode.go

jefchien · 2024-11-21T20:59:01Z

extension/agenthealth/config.go

@@ -12,6 +12,7 @@ import (
 type Config struct {
 	IsUsageDataEnabled bool              `mapstructure:"is_usage_data_enabled"`
 	Stats              agent.StatsConfig `mapstructure:"stats"`
+	StatusCodeOnly     *bool             `mapstructure:"is_status_code_only,omitempty"`


A bool with omitempty in the tag is enough.

cmd/amazon-cloudwatch-agent/amazon-cloudwatch-agent.go

extension/agenthealth/handler/stats/provider/interval.go

extension/agenthealth/handler/stats/agent/agent.go

extension/agenthealth/config.go

extension/agenthealth/handler/stats/agent/agent.go

extension/agenthealth/handler/stats/provider/statuscode.go

extension/agenthealth/handler/stats/handler.go

…deHandler

dricross · 2024-11-26T14:04:45Z

extension/agenthealth/handler/stats/agent/agent.go

@@ -104,6 +127,26 @@ func (of OperationsFilter) IsAllowed(operationName string) bool {
 	return of.allowAll || of.operations.Contains(operationName)
 }

+var StatusCodeOperations = []string{ // all the operations that are allowed


I think you're missing AssumeRole here

dricross · 2024-11-26T14:08:26Z

translator/translate/otel/extension/agenthealth/translator.go

@@ -59,6 +70,10 @@ func (t *translator) Translate(conf *confmap.Conf) (component.Config, error) {
 	if usageData, ok := common.GetBool(conf, common.ConfigKey(common.AgentKey, usageDataKey)); ok {
 		cfg.IsUsageDataEnabled = cfg.IsUsageDataEnabled && usageData
 	}
+	cfg.IsStatusCodeEnabled = t.statuscodeonly


Is this saying we only have status code when it's status code only? Why couldn't it do more than one thing? I don't know this package very well, but this behavior is slightly different than how "usage data" is enabled.

dricross · 2024-11-26T14:08:28Z

translator/translate/otel/extension/agenthealth/translator.go

@@ -59,6 +70,10 @@ func (t *translator) Translate(conf *confmap.Conf) (component.Config, error) {
 	if usageData, ok := common.GetBool(conf, common.ConfigKey(common.AgentKey, usageDataKey)); ok {
 		cfg.IsUsageDataEnabled = cfg.IsUsageDataEnabled && usageData
 	}
+	cfg.IsStatusCodeEnabled = t.statuscodeonly
+	if t.statuscodeonly != false && t.statuscodeonly {


redundant conditions

Suggested change

if t.statuscodeonly != false && t.statuscodeonly {

if t.statuscodeonly {

extension/agenthealth/handler/stats/handler.go

jefchien · 2024-11-27T16:40:15Z

extension/agenthealth/handler/stats/handler.go

+	}
+
+	if agentStatsEnabled {
+		clientStats := client.NewHandler(agent.NewOperationsFilter())


This changes the behavior of the client stats handler. None of the operations are allowlisted in this case because nothing is passed in.

jefchien · 2024-11-27T16:52:00Z

extension/agenthealth/handler/stats/agent/agent.go

+	}
+}
+
+func NewStatusCodeAndOtherOperationsFilter(operations []string) OperationsFilter {


Don't see a use case for this.

jefchien · 2024-11-27T16:52:57Z

extension/agenthealth/handler/stats/agent/agent.go

-	UsageFlags map[Flag]any `mapstructure:"usage_flags,omitempty"`
+// NewStatusCodeOperationsFilter creates a new filter for allowed operations and status codes.
+func NewStatusCodeOperationsFilter() OperationsFilter {
+	allowed := make(map[string]struct{}, len(StatusCodeOperations))


This is just an empty map that's the size of the StatusCodeOperations.

extension/agenthealth/extension.go

jefchien · 2024-11-27T17:01:37Z

extension/agenthealth/handler/stats/provider/process_test.go

@@ -77,6 +75,6 @@ func TestProcessStats(t *testing.T) {
 	mock.mu.Unlock()
 	provider.refresh()
 	assert.Eventually(t, func() bool {
-		return provider.getStats() == agent.Stats{}
+		return len(provider.getStats().StatusCodes) == 0 // map isn't comparable, so we check the length


Why is this checking the status codes? The point of this is to check that the stats it gets have been reset. If we're only checking the status codes, which are never set in this test, then it's a false positive.

This test I think is already flaky and is skipped but I added a function that will check if all attributes were reset.

extension/agenthealth/handler/stats/provider/statuscode.go

jefchien · 2024-11-27T17:05:48Z

extension/agenthealth/handler/stats/provider/statuscode.go

+			handler.filter = opsFilter
+		}
+		handler.startResetTimer()
+		statusCodeSingleton = handler


Are you sure we want to have the handler be a singleton? If we have locks, that means that every single API call the handler is attached to will be blocked until the locks are released. IMO the stats.Provider is the only one that needs to be a singleton. The handler doesn't. They can be separate.

extension/agenthealth/handler/stats/provider/statuscode.go

jefchien · 2024-11-27T18:11:51Z

extension/agenthealth/handler/stats/provider/statuscode.go

+	case "PutMetricData":
+		return "pmd"


Why is PutMetricData here?

fixing issue

2348639

Paramadon requested a review from a team as a code owner November 12, 2024 20:47

Paramadon added 19 commits November 12, 2024 16:01

trying to fix logs

b7fac5e

fixing issue

45536b8

adding debug statements (swapping logger with log.print)

75b024a

adding more log statments

b41de09

fixing tags

90a22a3

fixing logs

8c6d037

fixing logs

6ff99f8

adding a log

667f608

fixing stats function for statuscodehandler

9de9448

fixing issue

1e690e3

making operation names shorter

4c6adcf

Last commit works to reduce name- this one is for filtering operation…

dc129c9

…s from status codes

adding status code only filter

07a06fa

removing status code from otel config

0f76726

had to add return statments to filter

965ccdb

commenting out try configure for ec2tagger

a8268fd

adding ecw client for ec2 tagger

8ddb463

changing the middleware to metricID

3c15e81

trying to get stat

947b8f3

Paramadon changed the title ~~fixing issue~~ Adding Agent Health Extension Nov 15, 2024

Paramadon added 7 commits November 15, 2024 15:44

everything works just removing some debug statement and filter from h…

a76ef49

…andler

fixing filter issue

abf484b

previous commit works, just cleaning up debug statemtns

c849f61

adding unit tests

bd33dbc

fixing test

822680f

fixing unit tests

d547e1c

running make fmt

5dc40f4

Paramadon force-pushed the CodeHandler branch from 9465711 to 5dc40f4 Compare November 19, 2024 21:51

Paramadon added 3 commits November 19, 2024 18:02

removing internal changes

f64b332

restoring files

fa4c163

removing unnecessary file changes and passing unit test

457e48c

Paramadon changed the title ~~Adding Agent Health Extension~~ Adding Agent Health Extension Statuscode handler Nov 19, 2024

Paramadon changed the title ~~Adding Agent Health Extension Statuscode handler~~ Adding Agent Health Extension Statuscode Handler Nov 19, 2024

Paramadon changed the title ~~Adding Agent Health Extension Statuscode Handler~~ Adding Statuscode Handler to Agent Health Extension Nov 19, 2024

Paramadon added 3 commits November 19, 2024 18:12

cleaning up code

2705e87

fixing up pointer issue and passing in operations

3083626

adding agent health to prometheus

9e40bd8

mitali-salvi reviewed Nov 21, 2024

View reviewed changes

musa-asad reviewed Nov 21, 2024

View reviewed changes

tool/clean/clean_host/clean_host.go Outdated Show resolved Hide resolved

dricross reviewed Nov 21, 2024

View reviewed changes

jefchien reviewed Nov 21, 2024

View reviewed changes

Paramadon added 6 commits November 21, 2024 20:01

resolving comments

5745ee6

resolving comments

6e0fdd3

adding tests

8aee0d9

resolving comments

f344518

resolving comments

a7e0415

adding tests

d91706d

dricross reviewed Nov 22, 2024

View reviewed changes

extension/agenthealth/handler/stats/provider/statuscode.go Outdated Show resolved Hide resolved

Paramadon added 5 commits November 25, 2024 12:40

fixing race condition

59f735f

fixing formats

ebd92bd

fixing tests

44c4006

removing log statements

ecb4be5

fixed names

dfa3d4b

jefchien reviewed Nov 25, 2024

View reviewed changes

extension/agenthealth/handler/stats/handler.go Show resolved Hide resolved

Merge branch 'main' of github.com:aws/amazon-cloudwatch-agent into Co…

39e3a55

…deHandler

dricross reviewed Nov 26, 2024

View reviewed changes

jefchien reviewed Nov 27, 2024

View reviewed changes

jefchien mentioned this pull request Nov 27, 2024

Add Status Code Metrics for API Calls in the Agent #1442

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Statuscode Handler to Agent Health Extension #1423

Adding Statuscode Handler to Agent Health Extension #1423

Paramadon commented Nov 12, 2024 •

edited

Loading

mitali-salvi left a comment

mitali-salvi Nov 21, 2024

mitali-salvi Nov 21, 2024

jefchien Nov 21, 2024

dricross Nov 21, 2024

Paramadon Nov 21, 2024 •

edited

Loading

jefchien Nov 21, 2024

dricross Nov 26, 2024

dricross Nov 26, 2024

dricross Nov 26, 2024

jefchien Nov 27, 2024

jefchien Nov 27, 2024

jefchien Nov 27, 2024

jefchien Nov 27, 2024

Paramadon Dec 4, 2024

jefchien Nov 27, 2024

jefchien Nov 27, 2024

	if t.statuscodeonly != false && t.statuscodeonly {
	if t.statuscodeonly {

Adding Statuscode Handler to Agent Health Extension #1423

Are you sure you want to change the base?

Adding Statuscode Handler to Agent Health Extension #1423

Conversation

Paramadon commented Nov 12, 2024 • edited Loading

Description of the Issue

Changes Made

Impact

Testing

mitali-salvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paramadon Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paramadon commented Nov 12, 2024 •

edited

Loading

Paramadon Nov 21, 2024 •

edited

Loading