feat(logs): add support for fieldIndexPolicies in log group L2 Construct #33416

yashkh-amzn · 2025-02-12T18:14:59Z

Issue # (if applicable)

Reason for this change

Field Indexing for CloudWatch Logs (CWL) was launched in Nov 2024. A lot of CWL customers are asking for indexing support in L2 construct. This feature will enable that property under FieldIndexPolicies as a JSON object in the LogGroup construct.

Description of changes

The change here is just populating the fieldIndexPolicies property of the LogGroup CFN with the list of fields provided by the user. The format of this property will be like this:

const fieldIndexPolicy = new FieldIndexPolicy({
  fields: ['Operation', 'RequestId'],
});

new LogGroup(this, 'LogGroupLambda', {
  dataProtectionPolicy: dataProtectionPolicy,
  fieldIndexPolicies: [fieldIndexPolicy],
});

Describe any new or updated permissions being added

No new permissions have been added.

Description of how you validated changes

Added unit tests. Will add integ tests after getting a confirmation from the CDK team on the implementation.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

(This review is outdated)

yashkh-amzn · 2025-02-12T18:42:04Z

Clarification Request

aaythapa · 2025-02-13T18:18:12Z

Clarification Request

Hi @yashkh-amzn thank you for contributing!

I see the PR is missing integ tests and read me changes. To learn more about our integ test process and how to make them you can refer to this doc.

As for README changes they will presumably go here.

Note that until the linter and build passes this PR won't show up on our radar to review. Let us know if there is anything else you need clarifying.

yashkh-amzn · 2025-02-13T18:56:30Z

Clarification Request

Hi @yashkh-amzn thank you for contributing!

I see the PR is missing integ tests and read me changes. To learn more about our integ test process and how to make them you can refer to this doc.

As for README changes they will presumably go here.

Note that until the linter and build passes this PR won't show up on our radar to review. Let us know if there is anything else you need clarifying.

Hey @aaythapa, I had a sync up with your team during the CDK office hours. I was told that the CDK team first wanted to align with the business logic and hence it was okay to submit a PR first for an initial review. Once there would be an alignment, I can update the integ tests and README.
cc: @kaizencc @shikha372 @gracelu0

aaythapa · 2025-02-13T22:04:51Z

Clarification Request

Hi @yashkh-amzn thank you for contributing!
I see the PR is missing integ tests and read me changes. To learn more about our integ test process and how to make them you can refer to this doc.
As for README changes they will presumably go here.
Note that until the linter and build passes this PR won't show up on our radar to review. Let us know if there is anything else you need clarifying.

Hey @aaythapa, I had a sync up with your team during the CDK office hours. I was told that the CDK team first wanted to align with the business logic and hence it was okay to submit a PR first for an initial review. Once there would be an alignment, I can update the integ tests and README. cc: @kaizencc @shikha372 @gracelu0

Ah ok, thanks for letting me know. I'll bring this up with the team and wait for a response

shikha372 · 2025-02-18T02:30:34Z

packages/aws-cdk-lib/aws-logs/lib/log-group.ts

@@ -629,12 +637,23 @@ export class LogGroup extends LogGroupBase {
      Annotations.of(this).addWarningV2('@aws-cdk/aws-logs:propertyNotSupported', `The LogGroupClass property is not supported in the following regions: ${logGroupClassUnsupportedRegions}`);
    }

+    const fieldIndexPolicies: any[] = [];
+    if (props.fieldIndexPolicies) {
+      if (props.fieldIndexPolicies.length > 1) {


is this going to be changed in the near future ? if yes then we should remove this check from CDK else we won't know when it will be allowed to set more than 1.

It can be changed in the future but for now this condition does exist - https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_logs.CfnLogGroup.html#fieldindexpolicies. Can't we just remove this condition once we decide to support the array with multiple entries?

Will remove this condition and keep the check on the service side. Confirmed after testing that if we provide multiple field indexes without this condition, CFN fails:

shikha372 · 2025-02-18T02:30:52Z

packages/aws-cdk-lib/aws-logs/test/loggroup.test.ts

+  });
+});
+
+test('set field index policy positive test', () => {


what is the difference between this test and one above

Good catch! This was a dedup. I'll remove this test case.

shikha372 · 2025-02-18T02:35:21Z

packages/aws-cdk-lib/aws-logs/lib/field-index-policy.ts

+  private readonly fieldIndexPolicyProps: FieldIndexPolicyProps;
+
+  constructor(props: FieldIndexPolicyProps) {
+    if (props.fields.length > 20) {


is this something enforced through CFN handler ?, as it is not mentioned in the documentation
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#cfn-logs-loggroup-fieldindexpolicies

This is something which is enforced from the service side. From the official doc - https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/CloudWatchLogs-Field-Indexing-Syntax.html.

As many as 20 fields can be included in the policy.

shikha372 · 2025-02-18T02:37:52Z

packages/aws-cdk-lib/aws-logs/lib/log-group.ts

@@ -629,12 +637,23 @@ export class LogGroup extends LogGroupBase {
      Annotations.of(this).addWarningV2('@aws-cdk/aws-logs:propertyNotSupported', `The LogGroupClass property is not supported in the following regions: ${logGroupClassUnsupportedRegions}`);
    }

+    const fieldIndexPolicies: any[] = [];
+    if (props.fieldIndexPolicies) {


since this is allowed only for STANDARD log group, can we check the type of log group class.
Ref: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#cfn-logs-loggroup-fieldindexpolicies

Only log groups in the Standard log class support field index policies.

Sure thing, I can check that and update

Confirmed with the team that we can support Infrequent Access log groups in the future. So to keep the implementation consistent, we will not keep this condition to avoid having to make changes in the future and relying on the service side changes.

codecov · 2025-02-18T15:03:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.17%. Comparing base (47cae25) to head (8fc94cc).
Report is 24 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33416   +/-   ##
=======================================
  Coverage   82.17%   82.17%           
=======================================
  Files         119      119           
  Lines        6862     6862           
  Branches     1158     1158           
=======================================
  Hits         5639     5639           
  Misses       1120     1120           
  Partials      103      103

Flag	Coverage Δ
suite.unit	`82.17% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.17% <ø> (ø)`

aws-cdk-automation · 2025-02-20T00:11:50Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-21T00:11:40Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation · 2025-02-21T05:38:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8fc94cc
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

GavinZZ · 2025-02-25T20:52:32Z

Removing the reviewer-clarification-requested label as it's been clarified that README and Integ test will be added once an initial review was done by the team and the contributor has added them now.

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Feb 12, 2025

aws-cdk-automation requested a review from a team February 12, 2025 18:15

aws-cdk-automation previously requested changes Feb 12, 2025

View reviewed changes

aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Feb 12, 2025

yashkh-amzn changed the title ~~feat(aws-logs): Add support for fieldIndexPolicies in log group L2 Construct~~ feat(logs): add support for fieldIndexPolicies in log group L2 Construct Feb 12, 2025

aaythapa removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Feb 13, 2025

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 13, 2025

aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Feb 13, 2025

shikha372 reviewed Feb 18, 2025

View reviewed changes

yashkh-amzn requested a review from a team as a code owner February 21, 2025 03:59

yashkh-amzn added 2 commits February 20, 2025 21:04

Add support for fieldIndexPolicies in log group L2 Construct

4f62b1c

1. Add README & Integ tests 2. Address comments

8fc94cc

yashkh-amzn force-pushed the feature/loggroup-indexing branch from 1321c9d to 8fc94cc Compare February 21, 2025 05:08

GavinZZ removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Feb 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logs): add support for fieldIndexPolicies in log group L2 Construct #33416

feat(logs): add support for fieldIndexPolicies in log group L2 Construct #33416

yashkh-amzn commented Feb 12, 2025

aws-cdk-automation left a comment •

edited

Loading

yashkh-amzn commented Feb 12, 2025

aaythapa commented Feb 13, 2025

yashkh-amzn commented Feb 13, 2025

aaythapa commented Feb 13, 2025

shikha372 Feb 18, 2025 •

edited

Loading

yashkh-amzn Feb 18, 2025

yashkh-amzn Feb 20, 2025

shikha372 Feb 18, 2025

yashkh-amzn Feb 18, 2025

shikha372 Feb 18, 2025 •

edited

Loading

yashkh-amzn Feb 18, 2025

shikha372 Feb 18, 2025 •

edited

Loading

yashkh-amzn Feb 18, 2025

yashkh-amzn Feb 20, 2025

codecov bot commented Feb 18, 2025 •

edited

Loading

aws-cdk-automation commented Feb 20, 2025

aws-cdk-automation commented Feb 21, 2025

aws-cdk-automation commented Feb 21, 2025

GavinZZ commented Feb 25, 2025

feat(logs): add support for fieldIndexPolicies in log group L2 Construct #33416

Are you sure you want to change the base?

feat(logs): add support for fieldIndexPolicies in log group L2 Construct #33416

Conversation

yashkh-amzn commented Feb 12, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

yashkh-amzn commented Feb 12, 2025

aaythapa commented Feb 13, 2025

yashkh-amzn commented Feb 13, 2025

aaythapa commented Feb 13, 2025

shikha372 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

yashkh-amzn Feb 18, 2025

Choose a reason for hiding this comment

yashkh-amzn Feb 20, 2025

Choose a reason for hiding this comment

shikha372 Feb 18, 2025

Choose a reason for hiding this comment

yashkh-amzn Feb 18, 2025

Choose a reason for hiding this comment

shikha372 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

yashkh-amzn Feb 18, 2025

Choose a reason for hiding this comment

shikha372 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

yashkh-amzn Feb 18, 2025

Choose a reason for hiding this comment

yashkh-amzn Feb 20, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2025 • edited Loading

Codecov Report

aws-cdk-automation commented Feb 20, 2025

aws-cdk-automation commented Feb 21, 2025

aws-cdk-automation commented Feb 21, 2025

AWS CodeBuild CI Report

GavinZZ commented Feb 25, 2025

aws-cdk-automation left a comment •

edited

Loading

shikha372 Feb 18, 2025 •

edited

Loading

shikha372 Feb 18, 2025 •

edited

Loading

shikha372 Feb 18, 2025 •

edited

Loading

codecov bot commented Feb 18, 2025 •

edited

Loading