aws-cdk-lib: too many deprecation warnings issued. they can be deduplicated.

init-js commented 1 year ago

Describe the feature

We use aws-cdk-lib for deploying all of our stacks into production, and it emits north of 500 JSII deprecation warnings in some of our pipelines. I would like to request that the warnings be de-duplicated based on the position in the code, rather than based on the parameters passed in.

Those deprecation warnings are useful, we don't want to quiet them out. But I would argue that in many cases, the user doesn't call aws-cdk-lib directly, they'll use a series of library packages that assemble larger constructs for their CI.

Could we offer a choice to the users such that a given deprecation "annotation" can emit at most one warning in a given run?

When one deploys 50 different stacks in 50 different regions, the build output is just flooded with JSII deprecation warnings.

Use Case

The warnings arise because internal libraries we use have to support a wide range of aws-cdk-lib versions via peerDependencies (2.12.0 to 2.59.0). In many cases there's no way to get around a warning without doing some control flow branching based on the aws-cdk-lib version -- which is undesirable.

So we're stuck with the warnings until the libraries start dropping support for older versions of aws-cdk-lib.

Quieting the warnings themselves it too coarse of a control knob. These deprecation warnings do bring value, and in some cases, it's possible to update the user code (not the cdk middleware code) to work around them in a future-proof way.

Proposed Solution

Emit one warning per deprecation annotation.

Possible options (I'm sure there are more):

make the warning messages unique, and dedup based on warning messages
deduplicate warnings based on stack information (synthesize an error and inspect the err.stack).
add markers on the deprecation function to skip printing on subsequent invocations

Example code generated by JSII (in aws-cdk-lib/core/.jsii_warnings.js)

function _aws_cdk_core_ArnComponents(p) {
    if (p != null) {
        visitedObjects.add(p);
        try {
            visitedObjects.has(p.arnFormat) || (p.arnFormat, void 0), "sep" in p && print("@aws-cdk/core.ArnComponents#sep", "use arnFormat instead");
        } finally {
            visitedObjects.delete(p);
        }
    }
}

We could in this example, instead of marking each argument as visited, mark a new property on the function object...

...
if (!_aws_cdk_core_ArnComponents.visited) {
     _aws_cdk_core_ArnComponents.visited = true;
     print(....);
}
...

Other Information

I think this concern may have been overlooked in the rfc: https://github.com/aws/aws-cdk-rfcs/blob/master/text/287-cli-deprecation-warnings.md . I don't see that we explored solutions between a flood of warnings, and "quiet", but there are solutions midway in the space.

A workaround in our user code is to wrap console.warn and de-duplicate based on messages starting with [WARNING], which has been working okay. But more users would benefit if the deduplication happened inside aws-cdk-lib.

Another suggestion I can make (that's been very useful to us to resolve the deprecations) is to include a stack trace when touching a deprecated attribute, or invoking a deprecated function. This has allowed us to debug our construct libraries that wrap aws-cdk-lib. Having just the warning message isn't that useful when it happens mid-way through a giant synthesis operation -- whereas having the whole stack trace lets us hone in on the particular construct causing potential issues.

Acknowledgements

[ ] I may be able to implement this feature request
[ ] This feature might incur a breaking change

CDK version used

from 2.12.0 to 2.59.0 (peerDependencies aws-cdk-lib ^2.12.0)

Environment details (OS name and version, etc.)

linux AL2

peterwoodworth commented 1 year ago

Thanks for this feature request @init-js, this has good reasoning, and some good suggestions that could be fruitful. It would be great if we can support deduplication of warnings

peterwoodworth commented 1 year ago

Hey we just recently merged this PR, could you keep an eye out for the next release and let me know if this alleviates some or all of your duplicate warnings?

init-js commented 1 year ago

Fantastic! So as far as I understand it, it seems that each deprecation annotation node should would warn at most once now, without the user having to tweak any environment variables. Seems like that would solve 95% of the problem.

If we wanted to raise that bar further, we could include a stack trace logged with the warning message (without throwing an exception), but I can certainly live without that for now -- it's a nice-to-have. The trace is helpful in figuring out the offending part of the middleware that's hitting the deprecation.

I'll have to update my packages from 2.59.0 to 2.65.0 to try it out, hopefully that will go smoothly. But I will try it out! Code looks good otherwise.

Thanks all! Didn't expect it to be addressed so quickly.

github-actions[bot] commented 1 year ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

aws / aws-cdk