-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(apprunner): add ObservabilityConfiguration for AppRunner Service #30359
Conversation
4dbae36
to
a4e11b4
Compare
c4002bf
to
e73b10d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
Left some comments for some minor adjustments and an open question on the vendor
property.
@@ -1,3 +1,4 @@ | |||
// AWS::AppRunner CloudFormation Resources: | |||
export * from './service'; | |||
export * from './vpc-connector'; | |||
export * from './observability-configuration'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please order this alphabetically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I fixed it.
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Outdated
Show resolved
Hide resolved
/** | ||
* The implementation provider chosen for tracing App Runner services. | ||
* | ||
* @default Vendor.AWSXRAY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @default Vendor.AWSXRAY | |
* @default - tracing is not enabled for the service |
From the CloudFormation documentation, it seems we're allowed to omit this value.
Can you please verify if the construct deploys when that is the case?
Ie, changing the implementation to:
const resource = new CfnObservabilityConfiguration(this, 'Resource', {
observabilityConfigurationName: props.observabilityConfigurationName,
traceConfiguration: props.vendor ? {
vendor: props.vendor,
} : undefined,
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I fixed it, and I tried to deploy it.
An Observability Configuration without TraceConfiguration could be deployed.
% aws apprunner describe-observability-configuration --observability-configuration-arn arn:aws:apprunner:us-east-1: 01234567890:observabilityconfiguration/my-observability-no-vendor/1/2bc8fefaddcc4b878c0dfe2598412555 --region us-east-1
{
"ObservabilityConfiguration": {
"ObservabilityConfigurationArn": "arn:aws:apprunner:us-east-1:01234567890:observabilityconfiguration/my-observability-no-vendor/1/2bc8fefaddcc4b878c0dfe2598412555",
"ObservabilityConfigurationName": "my-observability-no-vendor",
"ObservabilityConfigurationRevision": 1,
"Latest": false,
"Status": "ACTIVE",
"CreatedAt": "2024-06-02T09:46:36.997000+09:00"
}
}
But the App Runner Service with it could not be deployed. It threw an error.
So, I added the above information to the documentation for the vendor property.
Co-authored-by: Luca Pizzini <[email protected]>
Co-authored-by: Luca Pizzini <[email protected]>
Co-authored-by: Luca Pizzini <[email protected]>
…uration.ts Co-authored-by: Luca Pizzini <[email protected]>
…uration.ts Co-authored-by: Luca Pizzini <[email protected]>
Co-authored-by: Luca Pizzini <[email protected]>
967bb81
to
8d7c747
Compare
8d7c747
to
b93cd9b
Compare
@@ -143,15 +145,25 @@ export class ObservabilityConfiguration extends cdk.Resource implements IObserva | |||
physicalName: props.observabilityConfigurationName, | |||
}); | |||
|
|||
if (props.observabilityConfigurationName !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a validation for observabilityConfigurationName and unit tests.
@lpizzinidev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍 Left comments for some final adjustments
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Outdated
Show resolved
Hide resolved
…uration.ts Co-authored-by: Luca Pizzini <[email protected]>
…uration.ts Co-authored-by: Luca Pizzini <[email protected]>
…uration.ts Co-authored-by: Luca Pizzini <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
packages/@aws-cdk/aws-apprunner-alpha/lib/observability-configuration.ts
Outdated
Show resolved
Hide resolved
…uration.ts Co-authored-by: Luca Pizzini <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
packages/@aws-cdk/aws-apprunner-alpha/test/obserbability-configuration.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for contributing!
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). |
@mazyu36 Can you fix the failed unit test please when available? |
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). |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
Closes #22985 .
Reason for this change
At the moment, L2 Construct does not support a tracing setting for the AppRunner Service.
Description of changes
ObservabilityConfiguration
ClassobservabilityConfiguration
property to theService
ClassDescription of how you validated changes
Add unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license