feat(metrics): add enhanced metrics support Co-authored-by: Brendan D…#711
Merged
Conversation
…oyle <brendansdoyle@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces comprehensive support for AppSync Enhanced Metrics configuration in the serverless-appsync-plugin, adding type definitions, validation rules, compilation logic, unit tests, and user documentation alongside a working example demonstrating the feature. ChangesEnhanced Metrics Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
examples/enhanced-metrics/schema.graphqlParsing error: '=' expected. Comment |
This was referenced May 30, 2026
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/enhanced-metrics.e2e.test.ts (1)
15-17: 💤 Low valueGuard
cleanup()against a failedbeforeAll.If
synthesize()throws inbeforeAll,resultremainsundefinedandafterAllwill throw aTypeError, masking the real synthesis error in the test output.♻️ Optional guard
afterAll(() => { - result.cleanup(); + result?.cleanup(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/enhanced-metrics.e2e.test.ts` around lines 15 - 17, The afterAll hook calls result.cleanup() unguarded so if synthesize() in beforeAll throws and result is undefined the afterAll will throw a TypeError; update the afterAll to check that result is defined (and optionally that result.cleanup is a function) before calling cleanup, and ensure the beforeAll use of synthesize() remains unchanged—look for the variables result, synthesize, beforeAll and afterAll in the test to apply the guard.src/resources/Resolver.ts (1)
34-41: AppSync ignores resolver-levelMetricsConfigwhenresolverLevelMetricsBehaviorisFULL_REQUEST_RESOLVER_METRICS, so defaulting to'DISABLED'is behavior-neutral in that mode.AWS AppSync documents that:
- With
FULL_REQUEST_RESOLVER_METRICS, enhanced metrics are emitted for all resolvers regardless of individual resolverMetricsConfig(so settingMetricsConfig: 'DISABLED'won’t opt a resolver out).- With
PER_RESOLVER_METRICS,MetricsConfigacts as the per-resolver filter (so defaulting to'DISABLED'whenmetricsConfigisn’t set is consistent with the intended behavior).Optional: you could omit emitting
MetricsConfigentirely whenresolverLevelMetricsBehaviorisFULL_REQUEST_RESOLVER_METRICSto reduce CloudFormation noise, but it won’t change runtime behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resources/Resolver.ts` around lines 34 - 41, The current block always sets Properties.MetricsConfig to a default 'DISABLED' whenever enhanced metrics are enabled, but AppSync ignores per-resolver MetricsConfig when resolverLevelMetricsBehavior is FULL_REQUEST_RESOLVER_METRICS; update the conditional in Resolver.ts so you only set Properties.MetricsConfig (using this.config.metricsConfig || 'DISABLED') when this.api.config.enhancedMetrics is true AND this.api.config.resolverLevelMetricsBehavior !== 'FULL_REQUEST_RESOLVER_METRICS' (or omit the property entirely in that mode) so CloudFormation noise is reduced and behavior remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/enhanced-metrics.e2e.test.ts`:
- Around line 15-17: The afterAll hook calls result.cleanup() unguarded so if
synthesize() in beforeAll throws and result is undefined the afterAll will throw
a TypeError; update the afterAll to check that result is defined (and optionally
that result.cleanup is a function) before calling cleanup, and ensure the
beforeAll use of synthesize() remains unchanged—look for the variables result,
synthesize, beforeAll and afterAll in the test to apply the guard.
In `@src/resources/Resolver.ts`:
- Around line 34-41: The current block always sets Properties.MetricsConfig to a
default 'DISABLED' whenever enhanced metrics are enabled, but AppSync ignores
per-resolver MetricsConfig when resolverLevelMetricsBehavior is
FULL_REQUEST_RESOLVER_METRICS; update the conditional in Resolver.ts so you only
set Properties.MetricsConfig (using this.config.metricsConfig || 'DISABLED')
when this.api.config.enhancedMetrics is true AND
this.api.config.resolverLevelMetricsBehavior !== 'FULL_REQUEST_RESOLVER_METRICS'
(or omit the property entirely in that mode) so CloudFormation noise is reduced
and behavior remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62523da9-2f34-4151-9afc-c3df16e74dfe
⛔ Files ignored due to path filters (1)
src/__tests__/validation/__snapshots__/base.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
doc/enhancedMetrics.mddoc/general-config.mde2e/enhanced-metrics.e2e.test.tsexamples/enhanced-metrics/schema.graphqlexamples/enhanced-metrics/serverless.ymlsrc/__tests__/api.test.tssrc/__tests__/resolvers.test.tssrc/__tests__/validation/base.test.tssrc/resources/Api.tssrc/resources/Resolver.tssrc/types/cloudFormation.tssrc/types/common.tssrc/types/index.tssrc/types/plugin.tssrc/validation.ts
AlexHladin
approved these changes
Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title:
feat(metrics): add enhanced metrics supportSupersedes #644. Original implementation by Brendan Doyle (@DoyleBrendan) — rebased onto current
master, corrected, and extended with tests. Because the branch diverged significantly (109+ commits, plus a jest 27 → 30 bump onmaster), this is opened as a fresh PR rather than an in-place update; the original work is credited via aCo-authored-bytrailer.What
Adds
appSync.enhancedMetrics, which maps to the AppSyncEnhancedMetricsConfigonAWS::AppSync::GraphQLApi, plus an optional per-resolvermetricsConfigso resolver-level metrics can actually be enabled.Correctness fixes on top of #644
OperationLevelMetricsConfigenum typo. The original value was' DISABLED'(leading space) in both the type and the validation enum. AWS's value isDISABLED, so the correct value was rejected by validation while the space variant would fail at deploy — the disabled path was unusable. Fixed everywhere.EnhancedMetricsConfigproperties as required, but the schema required none, soenhancedMetrics: {}passed validation and would synthesize an incomplete config that fails at deploy. The schema now requires all three (and disallows unknown keys).ResolverLevelMetricsBehaviorwas ignored. It was a required field in the public type but hardcoded toPER_RESOLVER_METRICSin the synthesized template. It's now honored from config.MetricsConfigchurn / non-functional feature. The original stampedMetricsConfig: 'DISABLED'onto every resolver unconditionally — changing the CloudFormation for every existing user on their next deploy, and (withPER_RESOLVER_METRICS+ always-DISABLED) making it impossible to ever enable resolver metrics. NowMetricsConfigis emitted only whenenhancedMetricsis configured, defaults toDISABLED, and is overridable per resolver via the newmetricsConfigfield. Stacks that don't use enhanced metrics get byte-identical CloudFormation.New public config
resolvers.<Type>.<field>.metricsConfig:'ENABLED' | 'DISABLED'(optional). Only emitted whenenhancedMetricsis set. This is the only new public surface and the main thing to review — it's what makesPER_RESOLVER_METRICSmeaningful.Tests
EnhancedMetricsConfigemitted with all three values when set; absent otherwise), resolver-level (omitted without enhanced metrics; defaults toDISABLED; honors per-resolver override), and validation (valid full config; missing required field and bad value rejected).examples/enhanced-metricsfixture +enhanced-metrics.e2e.test.tsasserting the synthesized template contains a completeEnhancedMetricsConfigand the expected per-resolverMetricsConfigvalues.npm run lintclean,npm test(296 passing),npm run test:e2e(88 passing across 31 suites).Reviewer notes / not covered here
serverless deployis the final word on the in-placeEnhancedMetricsConfig/MetricsConfigupdates.Co-authored-by: Brendan Doyle brendansdoyle@gmail.com
Summary by CodeRabbit
New Features
Documentation