Add support for 1-second Storage Resolution in the AWS EMF Exporter #36057

jpbarto · 2024-10-29T11:09:29Z

This change implements the capability for users of the AWS EMF Exporter to specify which metrics they would like to have sent to CloudWatch with a 1 second Storage Resolution. The EMF Exporter now explicitly states the Storage Resolution for each metric as 60 seconds, the previous implicit default, so there is no behavior change. If the user specifies a metric to have 1 second resolution it will be sent to CloudWatch EMF with the Storage Resolution set accordingly.

Description

Previously the AWS EMF Exporter sent metric data into CloudWatch without specifying the storage resolution. CloudWatch would then default to a 60 second storage resolution, even if metrics are sent more frequently than every 60 seconds. This would confuse users when they try to apply functions like AVG, SUM, MAX, or MIN to their metrics with a period of 5 seconds. The function would be applied by CloudWatch to 60 seconds worth of data and produced unexpected results and confusion for the user. This commit makes this 60 second resolution explicit in the messages sent to CloudWatch by the EMF Exporter and also gives the user the option to specify a more granular 1 second resolution per metric in the configuration file of the AWS EMF Exporter.

Link to tracking issue

Fixes #29506

Testing

Added tests to verify that config file parsing validates a metric descriptor that specifies either a valid unit, valid storage resolution, or both and rejects other invalid metric descriptors.
Added tests that the translation from metric data to CW EMF carries a storage resolution with it, defaulting to a value of 60 (current behavior) if no storage resolution valid is explicitly set in the configuration.

Documentation

Comments added in the code but have not updated the README.md pending acceptance of the PR.

…porter This change implements the capability for users of the AWS EMF Exporter to specify which metrics they would like to have sent to CloudWatch with a 1 second Storage Resolution. The EMF Exporter now explicitly states the Storage Resolution for each metric as 60 seconds, the previous implicit default, so there is no behavior change. If the user specifies a metric to have 1 second resolution it will be sent to CloudWatch EMF with the Storage Resolution set accordingly. Previously the AWS EMF Exporter sent metric data into CloudWatch without specifying the storage resolution. CloudWatch would then default to a 60 second storage resolution, even if metrics are sent more frequently than every 60 seconds. This would confuse users when they try to apply functions like AVG, SUM, MAX, or MIN to their metrics with a period of 5 seconds. The function would be applied by CloudWatch to 60 seconds worth of data and produced unexpected results and confusion for the user. This commit makes this 60 second resolution explicit in the messages sent to CloudWatch by the EMF Exporter and also gives the user the option to specify a more granular 1 second resolution per metric in the configuration file of the AWS EMF Exporter.

linux-foundation-easycla · 2024-10-29T11:09:34Z

The committers listed above are authorized under a signed CLA.

✅ login: jpbarto / name: Jason Barto (0bc7132, 2d92ca8, a4aeb7d, a29de76, ea995d4, 55a2416, 891e660, 111d623, 856c373, bb7e1a0)

Aneurysm9 · 2024-10-29T19:43:54Z

exporter/awsemfexporter/config.go

+	// Set the storage resolution for this metric in AWS Embedded Metric Format (EMF). Valid values are 1 (for 1 second resolution) and 60 (for 60 second resolution).
+	StorageResolution int `mapstructure:"storage_resolution"`


The MetricDescriptor configuration is extremely limited and can be accomplished through generally-applicable processors. I think it should be deprecated and slated for removal at some point in the future so we should not expand its surface area any further.

The latest update instead looks for a metric attribute named aws.emf.storage_resolution which can be applied either in the application's code or using the Attribute Processor in the collector itself.

Aneurysm9 · 2024-10-29T19:53:42Z

exporter/awsemfexporter/metric_translator.go

@@ -48,7 +48,7 @@ type cWMetrics struct {
 type cWMeasurement struct {
 	Namespace  string
 	Dimensions [][]string
-	Metrics    []map[string]string
+	Metrics    []map[string]any


I don't like the use of any here. Use the type system to prevent invalid data from being represented. If that means a new type needs to be created then a new type will need to be created. Rather than []map[string]any this should be something like []metricDefinition with metricDefinition defined like this:

type metricDefinition struct { Name string Unit string StorageResolution int }

The latest update creates a cWMetricInfo struct in the translator to mimic the metricInfo struct in grouped_metric.go, replacing the use of map[string]any in the translator.

This commit addresses comments in PR#36057 where it was suggested that metric description for the EMF Exporter should be deprecated and metric attributes used to specify the storage resolution for each metric. The code has been modified to remove the original changes and instead have the exporter look for a metric attribute named `aws.emf.storage_resolution` and use its value for the storage resolution when sending a message to CloudWatch EMF. The code has also added a new struct `cWMetricInfo` to hold the metric name, unit, and storage resolution where previously this was a `[]map[string]string` type. removed unnecessary changes

jpbarto · 2024-11-06T10:55:04Z

@Aneurysm9 are you able to approve the change in its current form which now uses metric attributes to define storage resolution?

mxiamxia · 2024-11-12T01:08:11Z

exporter/awsemfexporter/metric_translator.go

+
+	fmt.Printf("##--## Dim Set: %+v\n", dimSet)
+


Please remove the debug logs

mxiamxia · 2024-11-12T01:32:58Z

exporter/awsemfexporter/metric_translator.go

+	emfAttributeFilterRE := regexp.MustCompile(emfAttributeFilter)
+	for labelName := range labels {
+		if !emfAttributeFilterRE.MatchString(labelName) {
+			filteredLabels[labelName] = labels[labelName]
+		}
+	}


I'm concerned about the performance impact of applying regex matching on every metric attribute for each grouped metric data point.
Is there any requirement to add more aws.emf.* attribute patterns? Could you perform an exact match for labelname == "aws.emf.storage_resolution" and break the loop once it's found?

mxiamxia · 2024-11-12T01:34:03Z

exporter/awsemfexporter/metric_translator.go

 	dimensions := [][]string{dimSet}

+	fmt.Printf("##--## Dimensions: %+v", dimensions)


mxiamxia · 2024-11-12T01:34:11Z

exporter/awsemfexporter/metric_translator.go

@@ -228,14 +246,22 @@ func groupedMetricToCWMeasurement(groupedMetric *groupedMetric, config *Config)
 	// Add on rolled-up dimensions
 	dimensions = append(dimensions, rollupDimensionArray...)

-	metrics := make([]map[string]string, len(groupedMetric.metrics))
+	fmt.Printf("##--## Post Dimensions: %+v", dimensions)


mxiamxia · 2024-11-12T01:40:01Z

exporter/awsemfexporter/metric_translator.go

+
+	// metric attributes for AWS EMF, not to be treated as metric labels
+	// emfAttributeFilterRE          = regexp.MustCompile("^awsemf.[A-Za-z_]*$")
+	emfAttributeFilter            = "^awsemf.[A-Za-z_]*$"


q: why regex rule is ^awsemf.[A-Za-z_]*$ instead of ^aws.emf.[A-Za-z_]*$?

Thank you @mxiamxia - I've swapped the more complex regex for a simpler comparison operator. As new metric attributes are added this can be expanded to a switch statement which will likely still perform better than a compiled regex.

…erformant comparison operator

…ution' into awsemf_storageresolution

mxiamxia

LGTM. ty!

jpbarto · 2024-11-13T09:09:50Z

@andrzej-stencel good morning, are you able to review this pull request please?

andrzej-stencel · 2024-11-13T20:15:49Z

@jpbarto this probably deserves a changelog entry? Can you add one please https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#changelog.

…aws emf exporter

jpbarto · 2024-11-14T10:24:19Z

@andrzej-stencel the changelog has been updated as requested.

jpbarto · 2024-11-14T10:26:28Z

@andrzej-stencel @mxiamxia the PR requires a second reviewer to approve the pull request. This is currently assigned to @bryan-aguilar. Bryan was involved with the original issue raised last year and has been idle on GitHub since June of this year. Can I please ask that this PR be assigned to an alternate reviewer?

jpbarto requested a review from a team as a code owner October 29, 2024 11:09

jpbarto requested a review from andrzej-stencel October 29, 2024 11:09

github-actions bot assigned mwear Oct 29, 2024

github-actions bot added exporter/awsemf awsemf exporter internal/aws labels Oct 29, 2024

Aneurysm9 reviewed Oct 29, 2024

View reviewed changes

jpbarto force-pushed the awsemf_storageresolution branch from 04304bc to 8dc339a Compare October 31, 2024 15:45

jpbarto force-pushed the awsemf_storageresolution branch from 8dc339a to a29de76 Compare October 31, 2024 16:04

Merge branch 'main' into awsemf_storageresolution

856c373

Merge branch 'main' into awsemf_storageresolution

0bc7132

mxiamxia reviewed Nov 12, 2024

View reviewed changes

jpbarto added 2 commits November 12, 2024 10:42

Replaced the use of regex to match EMF metric attributes for a more p…

111d623

…erformant comparison operator

Merge remote-tracking branch 'refs/remotes/origin/awsemf_storageresol…

ea995d4

…ution' into awsemf_storageresolution

mxiamxia approved these changes Nov 12, 2024

View reviewed changes

Merge branch 'main' into awsemf_storageresolution

891e660

github-actions bot requested a review from bryan-aguilar November 13, 2024 09:05

andrzej-stencel approved these changes Nov 13, 2024

View reviewed changes

jpbarto added 2 commits November 14, 2024 09:49

Merge branch 'open-telemetry:main' into awsemf_storageresolution

55a2416

updated changelog to reflect new storage resolution feature added to …

bb7e1a0

…aws emf exporter

Merge branch 'main' into awsemf_storageresolution

a4aeb7d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for 1-second Storage Resolution in the AWS EMF Exporter #36057

Add support for 1-second Storage Resolution in the AWS EMF Exporter #36057

jpbarto commented Oct 29, 2024

linux-foundation-easycla bot commented Oct 29, 2024 •

edited

Loading

Aneurysm9 Oct 29, 2024

jpbarto Oct 31, 2024

Aneurysm9 Oct 29, 2024

jpbarto Oct 31, 2024

jpbarto commented Nov 6, 2024

mxiamxia Nov 12, 2024 •

edited

Loading

mxiamxia Nov 12, 2024

mxiamxia Nov 12, 2024

mxiamxia Nov 12, 2024

mxiamxia Nov 12, 2024 •

edited

Loading

jpbarto Nov 12, 2024

mxiamxia left a comment

jpbarto commented Nov 13, 2024

andrzej-stencel commented Nov 13, 2024

jpbarto commented Nov 14, 2024

jpbarto commented Nov 14, 2024

		// Set the storage resolution for this metric in AWS Embedded Metric Format (EMF). Valid values are 1 (for 1 second resolution) and 60 (for 60 second resolution).
		StorageResolution int `mapstructure:"storage_resolution"`

		dimensions := [][]string{dimSet}

		fmt.Printf("##--## Dimensions: %+v", dimensions)

Add support for 1-second Storage Resolution in the AWS EMF Exporter #36057

Are you sure you want to change the base?

Add support for 1-second Storage Resolution in the AWS EMF Exporter #36057

Conversation

jpbarto commented Oct 29, 2024

Description

Link to tracking issue

Testing

Documentation

linux-foundation-easycla bot commented Oct 29, 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

jpbarto commented Nov 6, 2024

mxiamxia Nov 12, 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

mxiamxia Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxiamxia left a comment

Choose a reason for hiding this comment

jpbarto commented Nov 13, 2024

andrzej-stencel commented Nov 13, 2024

jpbarto commented Nov 14, 2024

jpbarto commented Nov 14, 2024

linux-foundation-easycla bot commented Oct 29, 2024 •

edited

Loading

mxiamxia Nov 12, 2024 •

edited

Loading

mxiamxia Nov 12, 2024 •

edited

Loading