feat: log shadow mode through engine reporting API #3878
Conversation
packages/@lwc/integration-karma/test/accessibility/non-standard-aria-props/index.spec.js
Outdated
Show resolved
Hide resolved
|
/nucleus test |
|
Triggered a rerun of the karma tests this morning and they all pass now: https://github.com/salesforce/lwc/actions/runs/6962982231 |
|
|
|
We can merge as-is with the single reporting API and make the appropriate determinations on log vs incrementCounter at the app level. |
| mode: vm.shadowMode, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
I checked, and this gets tree-shaken if the customer is not using the reporting API! 🎉
| @@ -10,11 +14,11 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) { | |||
|
|
|||
| beforeEach(() => { | |||
| dispatcher = jasmine.createSpy(); | |||
| reportingControl.attachDispatcher(dispatcher); | |||
| attachReportingControlDispatcher(dispatcher, ['NonStandardAriaReflection']); | |||
| dispatch[1].tagName === 'x-aria-source' && | ||
| dispatch[1].attributeName === 'aria-labelledby' | ||
| ) | ||
| ).toHaveSize(2); |
There was a problem hiding this comment.
I think this is less clear than the previous (equivalent) code. I would prefer to change it back, but I also don't have a really strong opinion, since it's just test code.
There was a problem hiding this comment.
This variation is less strict about whether the dispatcher includes calls for other events. The previous code broke with the addition of new reporting events, while this one would not.
I'm fine changing it back since it's less critical to have that distinction with the new helper function. The previous behavior would still be asserting that the dispatcher is called twice for CrossRootAriaInSyntheticShadow AND zero times for NonStandardAriaReflection, but that's behavior less likely to break in the future.
There was a problem hiding this comment.
Yep that's exactly what I was thinking – let's use the new attachReportingControlDispatcher to control the filtering, rather than doing it here.
| tagName: 'x-component', | ||
| mode: process.env.NATIVE_SHADOW ? ShadowMode.Native : ShadowMode.Synthetic, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I think we should add a test here for CustomElementConstructor since it has its own slightly different code path (doesn't use lwc.createElement()).
It would also make sense to add a test for a light-DOM component, to ensure that it reports nothing.
packages/@lwc/integration-karma/test/mixed-shadow-mode/reporting/index.spec.js
Outdated
Show resolved
Hide resolved
Details
Adds telemetry for shadow mode usage (synthetic vs native).
Due to the volume of logs this is going to generate (easily TBs), we'll want to report this as a metric rather than as logs. This will require updating our API to distinguish between logs and metrics (e.g.
incrementCounter,log()). We may also want to refactor the existingreportAPI intolog.This aligns the runtime reporting with our compiler instrumentation:
https://github.com/salesforce/lwc/blob/6e75634e16a21c500d5e3f45400d1db3da3f5346/packages/%40lwc/errors/src/compiler/instrumentation.ts
Alternatively, we can make this decision in the consuming app container. (e.g.
if (reportName === 'ShadowModeUsage') { ... }). However, that would require each consuming app to know which reporting events require special handling. The app containers would also be taking a new dependency on the event names, which is not ideal.To make it easier to backport, we can introduce only the new API in patch and do the refactoring only in master.
Note: this should be done before LWR picks up their dependency to add reporting to the LWR container.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item