[RFC] Remove LogGroupName as default dimension

[jaredcnance]

This issue is to receive feedback on whether or not users want LogGroup as a default dimensions on your metrics. This was originally intended to enable deep-linking from metrics to the EMF events, but we no longer believe this is the correct approach to creating this linkage. This is a breaking change, so we want to hear your feedback.

aws-embedded-metrics-node/src/logger/MetricsLogger.ts

Lines 142 to 147 in 2ec8a84

	const defaultDimensions = {
	// LogGroup name will entirely depend on the environment since there
	// are some cases where the LogGroup cannot be configured (e.g. Lambda)
	LogGroup: environment.getLogGroupName(),
	ServiceName: Configuration.serviceName || environment.getName(),
	ServiceType: Configuration.serviceType || environment.getType(),

[cmckni3]

I agree with removing LogGroup as a default dimension. In my opinion, the log group adds noise to the CloudWatch metric.

[thantos]

Its creating more problem than value for me. CDK exposes the log groups name from lambda as /aws/lambda/[log group name], but this library creates the dimension as /[log group name] meaning my dashboard widgets won't work without some extra work (that I have not figured out yet).

[cmckni3]

Its creating more problem than value for me. CDK exposes the log groups name from lambda as /aws/lambda/[log group name], but this library creates the dimension as /[log group name] meaning my dashboard widgets won't work without some extra work (that I have not figured out yet).

It's tricky with CDK especially with CloudWatch dashboards. Would be interested in seeing your CDK stack/construct.

[thantos]

  metricCustomMetric(metricName: string, options?: MetricOptions): IMetric {
    return new Metric({
      namespace: MetricConstants.namespace,
      metricName,
      dimensions: {
        ServiceName: MetricConstants.serviceName(this.props.serviceQualifier),
        ServiceType: MetricConstants.serviceType,
        /*
         logGroupName will output /aws/lambda/[name]
         EMF will put just [name] in the log group name dimension. We need just [name].
        */
        LogGroup: Fn.select(3, Fn.split('/', this.computeMetricsFunction.logGroup.logGroupName))
      },
      ...options
    });
  }

[cmckni3]

@thantos Does the stack error out?

The code makes sense and seems like it would work.

A caveat with log groups and Lambda: log groups are created after the first invocation of the function. In my stack, I create the log group up front so I can attach metric filters.

[thantos]

No, I've recreated it a few times and the stack has been fine. I don't think metrics care about the existence of the metric its pointing to. And log name here is just a dimension and not an identifier.

[cmckni3]

No, I've recreated it a few times and the stack has been fine. I don't think metrics care about the existence of the metric its pointing to. And log name here is just a dimension and not an identifier.

Yeah. It depends. Metric filters and retention for log groups need the log group to be attached.

I started using CDK before the CFN retention property existed on log groups. Has been a journey.

[Glen-Moonpig]

Get rid 👍

[mduesterhoeft]

Actually the LogGroup dimension is getting in our way as well. We have metrics that are emitted from different lambdas. When adding alarms for such metrics we need to add alarms for each log group. This is a bit annoying and error prone. It would be desirable to at least be able to switch off the creation of the log group dimension.

[jensbodal]

FWIW you can use setDimension to override all of the default dimensions and just set your own.

// will not include "LogGroup","ServiceName","ServiceType" as dimensions and will only include path as a dimension
metricsLogger.setDimensions({ path });

[nicholas-wright-bjss]

My opinion is that the LogGroup, ServiceName and ServiceType dimensions should be opt-in rather than default. I have a scenario where multiple lambdas are using a service class that ideally would log metrics into a single place however they end up in multiple groups which makes my alarm configuration much more complicated.

Calling setDimensions(new DimensionSet()) before logging the metrics is our current workaround.

[lancegliser]

I'll agree to removing them as defaults. My naive understanding resulted in a CDK stack that looked like this:

// Register our metrics as alarms that hit the topic
if (areAlarmsEnabled) {
  alarms.push(
    new Alarm(this, "Metric-getDocuments", {
      comparisonOperator:
        ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
      threshold: 80000,
      evaluationPeriods: 1,
      alarmName: "Prism - API - GetDocument average duration",
      alarmDescription:
        "The average duration of getDocuments has exceeded the alarm condition.",
      metric: new Metric({
        namespace: cloudWatchMetricsNamespace,
        metricName: metricNameGetDocuments,
        unit: Unit.MICROSECONDS,
        statistic: "avg",
        period: Duration.minutes(5),
      }),
    })
  );

  const performanceTopic = new Topic(
    this,
    "Prism_Application_Api_Performance_Alarms"
  );
  // const emailAddress = new CfnParameter(this, `${id}-performance-email`);
  // myTopic.addSubscription(new EmailSubscription(emailAddress.valueAsString));
  performanceTopic.addSubscription(
    new EmailSubscription("***@torch.ai")
  );

  alarms.forEach((alarm) => {
    alarm.addAlarmAction(new SnsAction(performanceTopic));
  });
}

I had no idea why the metric was there, but not recording given my code for logging:

const metrics = createMetricsLogger();
metrics.putMetric(metricName, duration / 1000, Unit.Microseconds); // Nano to micro
await metrics.flush();

Knowing the dimensions could somehow cause aggregation failure was new to me upon first seeing this topic.