feat(sns): add TracingConfig prop by jun1-t · Pull Request #29715 · aws/aws-cdk

jun1-t · 2024-04-04T00:11:22Z

Issue # (if applicable)

Closes #29714

Reason for this change

Currently, to set the TracingConfig, it is necessary to configure it via L1.
So, add TracingConfig props to L2.

Description of changes

added TracingConfig props to topic.ts, sns.test.ts, integ.sns.ts, and README.md for AWS SNS.

Description of how you validated changes

I confirmed with unit test and integ test that it works as expected.

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

nmussy

Could you use an enum here, it makes for stronger typing, a simpler implementation for your PR, better DX, better documentation, etc

nmussy · 2024-04-04T03:37:48Z

packages/aws-cdk-lib/aws-sns/lib/topic.ts

+   * If PassThrough, the topic passes trace headers received from the Amazon SNS publisher to its subscription.
+   * If set to Active, Amazon SNS will vend X-Ray segment data to topic owner account if the sampled flag in the tracing header is true.
+   *
+   * @see https://docs.aws.amazon.com/sns/latest/dg/sns-active-tracing.html.


The period at the end breaks the URL

Suggested change

* @see https://docs.aws.amazon.com/sns/latest/dg/sns-active-tracing.html.

* @see https://docs.aws.amazon.com/sns/latest/dg/sns-active-tracing.html

nmussy · 2024-04-04T03:46:39Z

packages/aws-cdk-lib/aws-sns/lib/topic.ts

+   *
+   * @default PassThrough
+   */
+  readonly tracingConfig?: string;


An enumeration would be preferred here, since only two values are acceptable. Although I'm not sure why it's not properly documented in the CloudFormation docs, there should be a list of "Valid Values"

fixed to enum.

nmussy · 2024-04-04T03:46:42Z

packages/aws-cdk-lib/aws-sns/lib/topic.ts

+    if (
+      props.tracingConfig &&
+      !Token.isUnresolved(props.tracingConfig) &&
+      props.tracingConfig !== 'PassThrough' &&
+      props.tracingConfig !== 'Active'
+    ) {
+      throw new Error(`tracingConfig must be "PassThrough" or "Active", received: "${props.tracingConfig}".`);
+    }


The enumeration also allows your to remove this check

nmussy · 2024-04-04T03:47:09Z

packages/aws-cdk-lib/aws-sns/test/sns.test.ts

+    test('throw with incorrect tracingConfig', () => {
+      const app = new cdk.App();
+      const stack = new cdk.Stack(app);
+
+      expect(
+        () => new sns.Topic(stack, 'MyTopic', {
+          tracingConfig: 'Test',
+        }),
+      ).toThrow(/tracingConfig must be "PassThrough" or "Active", received: "Test"./);
+    });


This test can also be removed

nmussy · 2024-04-04T12:27:36Z

Did you forget to push your latest commits?

jun1-t · 2024-04-04T12:48:29Z

Did you forget to push your latest commits?

Failed build after code fix.
Please wait.

nmussy

Just some small stuff, LGTM otherwise 👍

nmussy · 2024-04-04T15:29:47Z

packages/aws-cdk-lib/aws-sns/lib/topic.ts

+  /**
+   * The mode that topic passes trace headers received from the Amazon SNS publisher to its subscription.
+   */
+  PASSTHROUGH = 'PassThrough',


Suggested change

PASSTHROUGH = 'PassThrough',

PASS_THROUGH = 'PassThrough',

nmussy · 2024-04-04T15:30:00Z

packages/aws-cdk-lib/aws-sns/lib/topic.ts

+   *
+   * @see https://docs.aws.amazon.com/sns/latest/dg/sns-active-tracing.html
+   *
+   * @default PassThrough


Suggested change

* @default PassThrough

* @default TracingConfig.PASS_THROUGH

nmussy · 2024-04-04T15:30:21Z

packages/aws-cdk-lib/aws-sns/README.md

+If PassThrough, the topic passes trace headers received from the Amazon SNS publisher to its subscription.
+If set to Active, Amazon SNS will vend X-Ray segment data to topic owner account if the sampled flag in the tracing header is true.
+
+The default TracingConfig is `PassThrough`.


Suggested change

The default TracingConfig is `PassThrough`.

The default TracingConfig is `TracingConfig.PASS_THROUGH`.

nmussy

LGTM, thanks for the contribution 👍

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

aws-cdk-automation · 2024-04-10T14:32:11Z

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation · 2024-04-10T14:57:38Z

AWS CodeBuild CI Report

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

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

jun1-t · 2024-04-10T15:31:49Z

@nmussy

LGTM, thanks for the contribution 👍

Thank you so much for your continued reviews.
However, I accidentally closed this pull request while trying to resolve conflicts.
I have opened a new pull request, could you please review it?
#29783

nmussy · 2024-04-10T15:34:43Z

No worries, I'll have a look 👍

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK feature-request A feature should be added or improved. p2 labels Apr 4, 2024

aws-cdk-automation requested a review from a team April 4, 2024 00:12

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 4, 2024

nmussy suggested changes Apr 4, 2024

View reviewed changes

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

nmussy suggested changes Apr 4, 2024

View reviewed changes

nmussy approved these changes Apr 6, 2024

View reviewed changes

github-actions bot mentioned this pull request Apr 8, 2024

Weekly PR metrics report #29758

Closed

jun1-t closed this Apr 10, 2024

jun1-t force-pushed the aws-sns_tracingConfig branch from b6dd635 to a7b4e29 Compare April 10, 2024 14:30

aws-cdk-automation requested changes Apr 10, 2024

View reviewed changes

nmussy mentioned this pull request Apr 10, 2024

feat(sns): add TracingConfig prop #29783

Merged

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

	* @see https://docs.aws.amazon.com/sns/latest/dg/sns-active-tracing.html.
	* @see https://docs.aws.amazon.com/sns/latest/dg/sns-active-tracing.html

	The default TracingConfig is `PassThrough`.
	The default TracingConfig is `TracingConfig.PASS_THROUGH`.

Conversation

jun1-t commented Apr 4, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Uh oh!

nmussy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmussy commented Apr 4, 2024

Uh oh!

jun1-t commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmussy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmussy left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Apr 10, 2024

Uh oh!

aws-cdk-automation commented Apr 10, 2024

AWS CodeBuild CI Report

Uh oh!

jun1-t commented Apr 10, 2024

Uh oh!

nmussy commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jun1-t commented Apr 4, 2024 •

edited

Loading