feat(logs): Add support for multiple parse and filter statements in QueryString #24022

MikeJamesWhite · 2023-02-06T01:15:56Z

Currently, QueryString is limited to only allow a single line/statement to be provided for each query command. For some commands this makes sense (e.g. limit), but for parse and filter this can be limiting. Adding multiple lines for these commands is possible in the AWS console, so it makes sense for it to be supported in CDK too.

In this PR, I'm adding support for filter and parse to be provided as string or string[], and adding/modifying various utility methods to handle this ambiguity.

I left the existing tests the same to verify no breaking changes, and added a new test for the newly enabled behavior.

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

…QueryString

gitpod-io · 2023-02-06T01:16:06Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

MikeJamesWhite · 2023-02-06T01:40:52Z

I made the changes requested by the bot above, however I don't have the ability to run the integ tests on my side. Would it be possible for the reviewer to execute these? Asking based on this guidance from the contribution guidelines:

If you are working on a PR that requires an update to an integration test and you are unable to run the cdk-integ tool to perform a real deployment, please call this out on the pull request so a maintainer can run the tests for you

Nevermind, managed to get the integ tests to run.

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

TheRealAmazonKendra · 2023-02-10T05:07:55Z

@Mergifyio update

mergify · 2023-02-10T05:08:15Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra

Thank you for your contribution! This looks great overall but there are a couple comments inline in regard to the types used.

TheRealAmazonKendra · 2023-02-10T05:15:08Z

packages/@aws-cdk/aws-logs/lib/query-definition.ts

  *
  * @default - no parse in QueryString
  */
-  readonly parse?: string;
+  readonly parse?: string | string[];


We can't use union types with jsii so this won't work/ Let's deprecate this prop and create a new on that's an optional string[]. Actually, can we also make this more strongly typed? If not, that's fine, but it would be nice to tighten the contract if we're deprecating props anyway.

Hey @TheRealAmazonKendra -- I'm not the most familiar with typescript or CDK so just checking if I understand: by more strongly typed do you mean something like adding a specific "ParseStatement" type which has "inputAttribute", "parsePattern" and "outputAttributes" as members? I could do this to ensure the generated line is in the correct format, and validation could even be added to catch user errors early at build time (e.g. when number of "*" in the parsePattern != length of outputAttributes).

However, I'm not sure if this would be too heavyweight (I would have assumed so, even though I don't mind spending the time to add it in), so interested in your thoughts?

I made the change to support string[] instead -- my feeling is that the above is too heavyweight anyway, but let me know :)

TheRealAmazonKendra · 2023-02-10T05:15:23Z

packages/@aws-cdk/aws-logs/lib/query-definition.ts

  *
  * @default - no filter in QueryString
  */
-  readonly filter?: string;
+  readonly filter?: string | string[];


Same comment as above.

I think making this one more strongly typed may be difficult, but happy to look into it if you have any suggestions :) Either way I can do the update to ensure jsii plays nicely, but will wait for your reply first.

Pull request has been modified.

comcalvi · 2023-02-15T17:59:30Z

packages/@aws-cdk/aws-logs/lib/query-definition.ts

+   * @param statements one or more query statements for the specified command, or undefined
+   * @returns an array of the query string lines generated from the provided command and statements
+   */
+  buildQueryLines(command: string, statements?: string[]): string[] {


should this be public? private? A function like noUndef was? This seems to be an internal-only thing that needs to hold no state, so I think it should be a function outside of the class

My feeling is that since it's irrelevant outside of the QueryString class, it should be a method of that class. That being said, I missed the private prefix so can definitely add that in.

comcalvi · 2023-02-15T18:04:16Z

packages/@aws-cdk/aws-logs/lib/query-definition.ts

    }
+
+    return (typeof statements === 'string' ? [statements] : statements).map(


statements is typed as string[] | undefined, so typeof statements === 'string' should never work. If it does work, then we've got our type modeling wrong. Please remove this check and ensure that only string[]s are passed to this function.

Forgot to update this after making changes following the previous comments -- pushing a change now :)

Pull request has been modified.

mergify · 2023-02-16T19:53:14Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-02-18T07:13:08Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-02-18T07:53:18Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-02-18T07:55:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 03e8ef0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

Add support for multiple parse and filter statements when creating a …

5bd97d8

…QueryString

aws-cdk-automation requested a review from a team February 6, 2023 01:16

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Feb 6, 2023

aws-cdk-automation previously requested changes Feb 6, 2023

View reviewed changes

MikeJamesWhite changed the title ~~feat(aws-logs): Add support for multiple parse and filter statements in QueryString~~ feat(logs): Add support for multiple parse and filter statements in QueryString Feb 6, 2023

Update README and integ test to cover the new behavior.

3654394

Run integ test.

581041c

Merge branch 'main' into main

f640969

TheRealAmazonKendra previously requested changes Feb 10, 2023

View reviewed changes

Addressing comments on PR.

02bfb5f

MikeJamesWhite requested a review from TheRealAmazonKendra February 14, 2023 17:31

comcalvi previously requested changes Feb 15, 2023

View reviewed changes

Addressing comments on PR.

9d3d1bf

MikeJamesWhite requested review from comcalvi and removed request for TheRealAmazonKendra February 15, 2023 19:38

comcalvi previously approved these changes Feb 16, 2023

View reviewed changes

Merge branch 'main' into main

fe96a6a

TheRealAmazonKendra approved these changes Feb 18, 2023

View reviewed changes

Merge branch 'main' into main

03e8ef0

mergify bot merged commit 75eb933 into aws:main Feb 18, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

WontonSam mentioned this pull request Oct 25, 2024

[Snyk] Upgrade @aws-cdk/assert from 1.128.0 to 2.68.0 WontonSam/apollo-federation-subgraph-compatibility#621

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logs): Add support for multiple parse and filter statements in QueryString #24022

feat(logs): Add support for multiple parse and filter statements in QueryString #24022

MikeJamesWhite commented Feb 6, 2023

gitpod-io bot commented Feb 6, 2023

aws-cdk-automation left a comment

MikeJamesWhite commented Feb 6, 2023 •

edited

Loading

TheRealAmazonKendra commented Feb 10, 2023

mergify bot commented Feb 10, 2023

TheRealAmazonKendra left a comment

TheRealAmazonKendra Feb 10, 2023

MikeJamesWhite Feb 10, 2023

MikeJamesWhite Feb 14, 2023

TheRealAmazonKendra Feb 10, 2023

MikeJamesWhite Feb 10, 2023

comcalvi Feb 15, 2023

MikeJamesWhite Feb 15, 2023

comcalvi Feb 15, 2023

MikeJamesWhite Feb 15, 2023

mergify bot commented Feb 16, 2023

mergify bot commented Feb 18, 2023

mergify bot commented Feb 18, 2023

aws-cdk-automation commented Feb 18, 2023

		}

		return (typeof statements === 'string' ? [statements] : statements).map(

feat(logs): Add support for multiple parse and filter statements in QueryString #24022

feat(logs): Add support for multiple parse and filter statements in QueryString #24022

Conversation

MikeJamesWhite commented Feb 6, 2023

gitpod-io bot commented Feb 6, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

MikeJamesWhite commented Feb 6, 2023 • edited Loading

TheRealAmazonKendra commented Feb 10, 2023

mergify bot commented Feb 10, 2023

✅ Branch has been successfully updated

TheRealAmazonKendra left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 16, 2023

mergify bot commented Feb 18, 2023

mergify bot commented Feb 18, 2023

aws-cdk-automation commented Feb 18, 2023

AWS CodeBuild CI Report

MikeJamesWhite commented Feb 6, 2023 •

edited

Loading