Adding Middleware to containerinsightrecieve/resourcedetection #265

Paramadon · 2024-12-09T00:49:36Z

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:

Tested on EKS by changing agent contrib to this branch.
Ex: replace github.com/open-telemetry/opentelemetry-collector-contrib/processor/resourcedetectionprocessor => github.com/amazon-contributing/opentelemetry-collector-contrib/processor/resourcedetectionprocessor v0.0.0-20241212025412-4bd7a1e9deed

…essors

dricross

We should have at least one unit test per client where the configurer is not nil to make sure it gets invoked. You can make the configurer something simple that sets a boolean and the test should check that the boolean is set by the configurer.

processor/resourcedetectionprocessor/config.go

processor/resourcedetectionprocessor/factory.go

processor/resourcedetectionprocessor/resourcedetection_processor.go

receiver/awscontainerinsightreceiver/config.go

receiver/awscontainerinsightreceiver/internal/host/ebsvolume.go

Paramadon · 2024-12-11T17:09:10Z

receiver/awscontainerinsightreceiver/internal/host/hostinfo.go

-		ec2MetadataCreator:    newEC2Metadata,
-		ebsVolumeCreator:      newEBSVolume,
-		ec2TagsCreator:        newEC2Tags,
+		ec2MetadataCreator: withConfigurer(configurer, func(c *awsmiddleware.Configurer) func(context.Context, *session.Session, time.Duration, chan bool, chan bool, bool, int, *zap.Logger, ...ec2MetadataOption) ec2MetadataProvider {


Here's what is happening here:
a. The configurer is passed to withConfigurer.

b. A function is defined that takes a *awsmiddleware.Configurer and returns another function. This returned function is the actual EC2 metadata creator function.

c. The inner function (the EC2 metadata creator) now has access to the Configurer (as c) when it's called, allowing it to use the configurer when creating the EC2 metadata provider.

d. The newEC2Metadata function is called with all the parameters plus the Configurer (c).

This pattern allows the Configurer to be injected into the EC2 metadata creation process without changing the signature of the ec2MetadataCreator function that's stored in the Info struct.

I think we need to break that line down and explain whats going on using helper functions, because currently you it is difficult to understand whats going on there without coming to this PR and finding this comment

same thing with the return functions down below.

musa-asad · 2024-12-11T18:00:09Z

Can you update the description to describe how it differs between #265 just for clarity more than just the title?

internal/metadataproviders/aws/ec2/metadata.go

okankoAMZ

Looks good!

internal/metadataproviders/aws/ec2/metadata.go

processor/resourcedetectionprocessor/go.mod

jefchien · 2024-12-16T17:02:28Z

processor/resourcedetectionprocessor/internal/aws/ec2/ec2.go

+func (d *Detector) ExposeHandlers(ctx context.Context) (handlers *request.Handlers) {
+	return d.metadataProvider.GetHandlers(ctx)
+}


nit: For consistency and to make it easier to follow, keep the names of the functions used to access the request handlers the same. Would suggest keeping it the same as the internal/aws/cwlogs client.

opentelemetry-collector-contrib/internal/aws/cwlogs/cwlog_client.go

Lines 89 to 91 in 66e9942

func (client *Client) Handlers() *request.Handlers {

return &client.svc.(*cloudwatchlogs.CloudWatchLogs).Handlers

}

processor/resourcedetectionprocessor/internal/resourcedetection.go

jefchien · 2024-12-16T18:54:09Z

processor/resourcedetectionprocessor/internal/resourcedetection.go

+func (p *ResourceProvider) ConfigureHandlers(ctx context.Context, host component.Host, middlewareId component.ID) {
+	for _, detector := range p.detectors {
+		if handlerDetector, ok := detector.(ExposeHandlerDetector); ok {
+			awsmiddleware.TryConfigure(p.logger, host, middlewareId, awsmiddleware.SDKv1(handlerDetector.ExposeHandlers(ctx)))


nit: Could grab the Configurer once and pass that into this function instead of the host and middlewareID so it isn't trying to get it each time.

jefchien · 2024-12-16T19:02:09Z

receiver/awscontainerinsightreceiver/internal/host/hostinfo.go

+func createEC2MetadataCreator(configurer *awsmiddleware.Configurer) func(context.Context, *session.Session, time.Duration, chan bool, chan bool, bool, int, *zap.Logger, ...ec2MetadataOption) ec2MetadataProvider {
+	return withConfigurer(configurer, func(c *awsmiddleware.Configurer) func(context.Context, *session.Session, time.Duration, chan bool, chan bool, bool, int, *zap.Logger, ...ec2MetadataOption) ec2MetadataProvider {
+		return func(ctx context.Context, session *session.Session, refreshInterval time.Duration, instanceIDReadyC, instanceIPReadyC chan bool, localMode bool, imdsRetries int, logger *zap.Logger, options ...ec2MetadataOption) ec2MetadataProvider {
+			return newEC2Metadata(ctx, session, refreshInterval, instanceIDReadyC, instanceIPReadyC, localMode, imdsRetries, logger, c, options...)
+		}
+	})
+}


Why do we need any of this? Can't we just change the functions to take in the configurer?

adding handlers to containerinsightreciever and resourcedetectionproc…

d63bbf4

…essors

Paramadon requested a review from mxiamxia as a code owner December 9, 2024 00:49

Paramadon added 3 commits December 9, 2024 20:32

reverting exporter changes

9dce4e2

trying to fix errors

8a9b9f3

restoring file

dbb6bb5

Paramadon changed the title ~~adding handlers to containerinsightreciever and resourcedetectionproc…~~ Adding Middleware to containerinsightrecieve/resourcedetection Dec 10, 2024

dricross reviewed Dec 11, 2024

View reviewed changes

Paramadon commented Dec 11, 2024

View reviewed changes

musa-asad reviewed Dec 11, 2024

View reviewed changes

internal/metadataproviders/aws/ec2/metadata.go Show resolved Hide resolved

added unit test for clients

87208dd

dricross previously approved these changes Dec 12, 2024

View reviewed changes

adding helper functions and comments

c223485

Paramadon dismissed dricross’s stale review via c223485 December 16, 2024 18:22

okankoAMZ previously approved these changes Dec 16, 2024

View reviewed changes

jefchien reviewed Dec 16, 2024

View reviewed changes

resolving comments

5dcbb54

Paramadon dismissed okankoAMZ’s stale review via 5dcbb54 December 16, 2024 19:45

okankoAMZ approved these changes Dec 16, 2024

View reviewed changes

dricross approved these changes Dec 16, 2024

View reviewed changes

Paramadon merged commit 2dd9562 into aws-cwa-dev Dec 16, 2024
125 of 146 checks passed

jefchien reviewed Dec 16, 2024

View reviewed changes

Paramadon mentioned this pull request Dec 17, 2024

Upgrade otel compenents to v0.0.0-20240528184218-531527333157 aws/amazon-cloudwatch-agent#1473

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Middleware to containerinsightrecieve/resourcedetection #265

Adding Middleware to containerinsightrecieve/resourcedetection #265

Paramadon commented Dec 9, 2024 •

edited

Loading

dricross left a comment

Paramadon Dec 11, 2024

okankoAMZ Dec 12, 2024

okankoAMZ Dec 12, 2024

musa-asad commented Dec 11, 2024 •

edited

Loading

okankoAMZ left a comment

jefchien Dec 16, 2024

jefchien Dec 16, 2024

jefchien Dec 16, 2024

	func (client Client) Handlers() request.Handlers {
	return &client.svc.(*cloudwatchlogs.CloudWatchLogs).Handlers
	}

Adding Middleware to containerinsightrecieve/resourcedetection #265

Adding Middleware to containerinsightrecieve/resourcedetection #265

Conversation

Paramadon commented Dec 9, 2024 • edited Loading

Description of the Issue

Changes Made

Impact

Testing

dricross left a comment

Choose a reason for hiding this comment

Paramadon Dec 11, 2024

Choose a reason for hiding this comment

okankoAMZ Dec 12, 2024

Choose a reason for hiding this comment

okankoAMZ Dec 12, 2024

Choose a reason for hiding this comment

musa-asad commented Dec 11, 2024 • edited Loading

okankoAMZ left a comment

Choose a reason for hiding this comment

jefchien Dec 16, 2024

Choose a reason for hiding this comment

jefchien Dec 16, 2024

Choose a reason for hiding this comment

jefchien Dec 16, 2024

Choose a reason for hiding this comment

Paramadon commented Dec 9, 2024 •

edited

Loading

musa-asad commented Dec 11, 2024 •

edited

Loading