Conversation
movence
added
the
ready for testing
| "metric_declaration": [ | ||
| { | ||
| "source_labels": ["job"], | ||
| "label_matcher": "^kubernetes-.*", | ||
| "dimensions": [["job"], ["job", "instance"]], | ||
| "metric_selectors": [ | ||
| "^container_.*", | ||
| "^pod_.*" | ||
| ] | ||
| } | ||
| ] |
There was a problem hiding this comment.
Did we validate that the metric_declarations work as expected? Just need to make sure that the EMF exporter isn't normalizing any of the metric attributes.
| otlpDefaultLogGroupFormat = "/aws/cwagent" | ||
| ) | ||
|
|
||
| func setOTLPLogGroup(conf *confmap.Conf, cfg *awsemfexporter.Config) error { |
There was a problem hiding this comment.
nit: Why does this return an error if it's always nil?
| func setOTLPNamespace(conf *confmap.Conf, cfg *awsemfexporter.Config) error { | ||
| if namespace, ok := common.GetString(conf, common.ConfigKey(otlpEMFProcessorBasePathKey, metricNamespace)); ok { | ||
| cfg.Namespace = namespace | ||
| return nil | ||
| } | ||
|
|
||
| cfg.Namespace = otlpDefaultCloudWatchNamespace | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: Could create a common.GetOrDefaultString similar to common.GetOrDefaultBool to simplify this a bit.
| func setOTLPMetricDescriptors(conf *confmap.Conf, cfg *awsemfexporter.Config) error { | ||
| metricUnitKey := common.ConfigKey(otlpEMFProcessorBasePathKey, metricUnit) | ||
| if !conf.IsSet(metricUnitKey) { | ||
| return nil | ||
| } | ||
|
|
||
| mus := conf.Get(metricUnitKey) | ||
| metricUnits := mus.(map[string]interface{}) | ||
| var metricDescriptors []map[string]string | ||
| for mName, unit := range metricUnits { | ||
| metricDescriptors = append(metricDescriptors, map[string]string{ | ||
| "metric_name": mName, | ||
| "unit": unit.(string), | ||
| }) | ||
| } | ||
| c := confmap.NewFromStringMap(map[string]interface{}{ | ||
| "metric_descriptors": metricDescriptors, | ||
| }) | ||
| cfg.MetricDescriptors = []awsemfexporter.MetricDescriptor{} | ||
| if err := c.Unmarshal(&cfg); err != nil { | ||
| return fmt.Errorf("unable to unmarshal metric_descriptors: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: This is exactly the same as setPrometheusMetricDescriptors can we combine it?
| func setOTLPMetricDeclarations(conf *confmap.Conf, cfg *awsemfexporter.Config) error { | ||
| metricDeclarationKey := common.ConfigKey(otlpEMFProcessorBasePathKey, metricDeclartion) | ||
| if !conf.IsSet(metricDeclarationKey) { | ||
| return nil | ||
| } | ||
| metricDeclarations := conf.Get(metricDeclarationKey) | ||
| var declarations []map[string]interface{} | ||
| for _, md := range metricDeclarations.([]interface{}) { | ||
| metricDeclaration := md.(map[string]interface{}) | ||
| declaration := map[string]interface{}{} | ||
| if dimensions, ok := metricDeclaration["dimensions"]; ok { | ||
| declaration["dimensions"] = dimensions | ||
| } | ||
| if metricSelectors, ok := metricDeclaration["metric_selectors"]; ok { | ||
| declaration["metric_name_selectors"] = metricSelectors | ||
| } else { | ||
| // If no metric selectors are provided, that particular metric declaration is invalid | ||
| continue | ||
| } | ||
| labelMatcher, ok := metricDeclaration["label_matcher"] | ||
| if !ok { | ||
| labelMatcher = ".*" | ||
| } | ||
| sourceLabels, ok := metricDeclaration["source_labels"] | ||
| if ok { | ||
| // OTel awsemfexporter allows specifying multiple label_matchers but CWA only allows specifying one | ||
| declaration["label_matchers"] = [...]map[string]interface{}{ | ||
| { | ||
| "label_names": sourceLabels, | ||
| "regex": labelMatcher, | ||
| }, | ||
| } | ||
| } else { | ||
| // If no source labels or label matchers are provided, that particular metric declaration is invalid | ||
| continue | ||
| } | ||
| declarations = append(declarations, declaration) | ||
| } | ||
| c := confmap.NewFromStringMap(map[string]interface{}{ | ||
| "metric_declarations": declarations, | ||
| }) | ||
| cfg.MetricDeclarations = []*awsemfexporter.MetricDeclaration{} // Clear out any existing declarations | ||
| if err := c.Unmarshal(&cfg); err != nil { | ||
| return fmt.Errorf("unable to unmarshal metric_declarations: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: Same as the other one. Exactly the same as the prometheus version. Could have a shared helper function.
| } | ||
|
|
||
| func setOTLPFields(conf *confmap.Conf, cfg *awsemfexporter.Config) error { | ||
| setDisableMetricExtraction(otlpBasePathKey, conf, cfg) |
There was a problem hiding this comment.
nit: I know it's not part of this PR, but this function name feels misleading. It's disabling DisableMetricExtraction, but it made me think this was disabling metric extraction.
| return conf.IsSet(common.OTLPLogsKey) | ||
| } | ||
|
|
||
| func isOTLPEMF(conf *confmap.Conf, pipelineName string) bool { |
There was a problem hiding this comment.
OTLP should have its own awsemfexporter. What if metric declarations are set for OTLP? Won't other metrics be impacted that are sent by the shared EMF exporter?
There was a problem hiding this comment.
OTLP already gets its own emf exporter so there is no impact to other emf exporters (prom, CI or etc)
| }, | ||
| "log_group_name": { | ||
| "description": "CloudWatch log group name for OTLP metrics", | ||
| "type": "string" |
There was a problem hiding this comment.
Should probably place a restriction on length like for the other fields
There was a problem hiding this comment.
Yeah I can make it to use already existing logGroupNameDefinition but the current schema is already inconsistent where prometheus contains log_group_name without any restrictions. Maybe it's better to use logGroupNameDefinition everywhere but this might affect agent configs already using longer names with prometheus.
There was a problem hiding this comment.
Doesn't have to be in this PR, but based on documentation https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_CreateLogGroup.html
Log group names can be between 1 and 512 characters long.
| }, | ||
| "log_group_name": { | ||
| "description": "CloudWatch log group name for OTLP metrics", | ||
| "type": "string" |
There was a problem hiding this comment.
Doesn't have to be in this PR, but based on documentation https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_CreateLogGroup.html
Log group names can be between 1 and 512 characters long.
| imds_retries: 1 | ||
| local_mode: false | ||
| log_group_name: /aws/cwagent | ||
| log_group_name: /aws/application/otlp |
There was a problem hiding this comment.
nit: Could we have a test that shows it fallsback to defaults if not configured or does that already exist in a different sample config?
There was a problem hiding this comment.
nit: So the EMF exporter is unnamed. Do we want to name it (awsemf/otlp) so we don't accidentally share the exporter instance in the future with other pipelines?
There was a problem hiding this comment.
Why was this added? Why do we need this now when it wasn't required before?
| } | ||
|
|
||
| // setNamespaceWithDefault is a shared function to set namespace from config or use a default | ||
| func setNamespaceWithDefault(conf *confmap.Conf, namespaceKey string, defaultNamespace string, cfg *awsemfexporter.Config) error { |
There was a problem hiding this comment.
nit: Why does this return an error if it's always nil?
|
This PR was marked stale due to lack of activity. |
3 participants
Description of the issue
Agent does not allow customers to customize EMF (Embedded Metric Format) logging settings when using OTLP metrics
Description of changes
Add support for
log_group_nameandemf_processorblurbLicense
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Using agent config LogGroup="OtlpEMFTest" AND MetricNamespace=OTLPEMF/Test"
With a dummy bash sending EMF logs via OTLP
Requirements
Before commiting your code, please do the following steps.
make fmtandmake fmt-shmake lintIntegration Tests
To run integration tests against this PR, add the
ready for testinglabel.