Skip to content

feat(devtools): create profiler for injector events in dev mode#48639

Closed
AleksanderBodurri wants to merge 4 commits intoangular:mainfrom
AleksanderBodurri:track-injected-dependencies
Closed

feat(devtools): create profiler for injector events in dev mode#48639
AleksanderBodurri wants to merge 4 commits intoangular:mainfrom
AleksanderBodurri:track-injected-dependencies

Conversation

@AleksanderBodurri
Copy link
Copy Markdown
Member

@AleksanderBodurri AleksanderBodurri commented Jan 4, 2023

See individual commits

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jan 4, 2023
@ngbot ngbot bot added this to the Backlog milestone Jan 19, 2023
@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch 6 times, most recently from 7ae67df to 8309909 Compare May 22, 2023 20:54
@AleksanderBodurri AleksanderBodurri marked this pull request as ready for review May 22, 2023 20:56
@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch from 8309909 to 9aa4ed2 Compare May 22, 2023 20:59
@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch from 9aa4ed2 to 358bfeb Compare May 22, 2023 21:12
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AleksanderBodurri great work! 👍

A couple high-level comments:

  • it'd be helpful to add more comments into the code to cover function contracts as well as some tricky parts of the logic
  • it'd be really awesome to minimize the footprint of the debug-related code in the main/production code, so it's easier to follow and see where dev mode starts and ends
  • some functions would benefit from refactoring to split them into smaller fns

Thank you.

@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch 3 times, most recently from 2b37766 to 9194965 Compare May 24, 2023 01:26
@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch 11 times, most recently from 11a7675 to 2e5205e Compare May 30, 2023 15:41
@AndrewKushnir
Copy link
Copy Markdown
Contributor

FYI, the PR is no longer blocked on the internal Google tests (the necessary code is cleaned up).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these docs would be ideally on the interface and its fields, so they show up in intellisense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still applies :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a class instead? It seems to be instantiated as one, with all the fields initialized to an empty map.

Also minor nit: we typically use camel case for acronyms, so this would be DiData.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This anonymous function seems important enough to be extracted and named.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than destructuring the fields of InjectorProfilerEvent, we should set it up as a type union so that by narrowing the type we get correct typing for data. That would ensure we get type safety without the casts.

Additionally, data should be renamed to something more specific for each event type (like service for the InjectedService in the case of an Inject event).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can context be undefined here? This should probably be context.injector.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above we should break this into specific interfaces and make InjectorProfilerEvent a type union of them, discriminated by the type.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this wrapper? Its various getters could just be implemented as utility functions that take a NodeInjector.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining what running providerToFactory in this context is actually doing? That operation doesn't perform any instantiation or interact with the DI profiler at all, as far as I can tell.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: prefer ?? to || (yay modern JavaScript)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be a logic bug here.

It's entirely possible that dep was injected with the SkipSelf flag, meaning that even if the first injector in the resolution path had that token, it wouldn't have matched. But this search doesn't consider dep.flags, so it would assume that dep came from the first injector.

We could take the flags into consideration here (including Host). Another approach would be to actually capture the injector from which a dependency came at the time that dependency was injected.

@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch 2 times, most recently from 94f339e to b5512fe Compare July 28, 2023 07:09
Currently, understanding dependency injection in Angular requires a lot of context and has been sited in our surveys as one of the largest points of confusion that our users have with the framework. This commit is the beginning of our approach to make debugging dependency injection in Angular easier.

This commit introduces injector profiler callbacks in parts of the framework to emit injector events. It also introduces a default handler for these events. This default handler parses the stream of events to construct some data structures that will support new injector debug APIs.

We have implemented a similar pattern in the past to minimize overhead. There is also the possiblity of making the internal `setInjectorProfiler` function a public debug API in the future, so that users can implement their own handlers to debug DI events. Lastly DI in Angular maps nicely to a stream of events.

For production applications there is no runtime overhead. For applications in dev mode there is some additional overhead from the default profiler handling injector events and holding debug data in memory.

For production applications, dead code elimination should strip all of the code used by this PR.
…hProviders` to callback style interfaces

walkProviderTree and processInjectorTypesWithProviders both perform some generic traversal logic of the import graph of an input NgModule or Standalone component. Currently, these functions pass around a `providersOut` array that is used to collect providers at each step of the traversal.

This PR converts those functions to accept visitor callbacks instead of the `providersOut` array. This is done to make the traversal logic of these functions reusable, while leaving it up to the visitor to determine the logic that fires for each visited node.

This refactor would allow us to reuse `walkProviderTree` for injector debugging APIs that could support some cool features in DevTools, like tracing the injector resolution path of an injected property on a component instance all the way up to the specific imported module/standalone component.
This commit introduces 3 new APIs.

getDependenciesFromInstantiation:
- Given an injector and a token that was instantiated by that injector, discover all of the things were injected in the instance of that token
- This API is meant to enable recursive inspection of dependencies. Dependencies returned by this API include which injector they were providedIn, which enables the continous use of getDependenciesFromInstantiation to determine the dependencies of dependencies

getInjectorProviders:
- Given an injector, discover all of the providers that were configured in that injector.
- This API returns information on the configured providers of an injector, including the import path that leads to the container that the provider originated from (NgModule or standalone directive). This enables fine grained inspection to determine where a specific provider comes from.

getInjectorParent:
- Given an injector, discover the parent of that injector.
- This function is meant to be used recursively to discover the entire resolution path from a starting injector to the NullInjector.

These APIs were designed to be used together. For example, getInjectorParent can be used to discover the structure of an injector hierarchy. Once that's done, getInjectorProviders can be used to determine the providers of each injector in that hierarchy.

Another example: getDependenciesFromInstantiation can be used to discover the dependencies of a specific injector constructed instance. From there, we can use getInjectorParent to discover the injector resolution path and map each dependency to a path from the starting injector to the injector that it was provided in.
…ng APIs

    Creates unit tests for the following APIs

    - setInjectorProfiler
    - getInjectorProviders
    - getInjectorResolutionPath
    - getDependenciesFromInjectable

    Modifies existing tests in

    - packages/examples/core/di/ts/injector_spec.ts
    - packages/core/test/render3/jit/declare_injectable_spec.ts
    - packages/core/test/render3/jit/declare_factory_spec.ts

    because they setup framework injector context manually.

    Exports setInjectorProfilerContext in packages/core/src/core_private_export.ts in order for use
    in the the modified tests above.
@AleksanderBodurri AleksanderBodurri force-pushed the track-injected-dependencies branch from b5512fe to 94dc0f2 Compare July 28, 2023 07:15
@AndrewKushnir AndrewKushnir requested a review from alxhub July 29, 2023 00:34
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Global Presubmit.

@AndrewKushnir
Copy link
Copy Markdown
Contributor

FYI, the Global Presubmit is "green".

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels Jul 31, 2023
// these are the only providers that do not go through the value hydration logic
// where this event would normally be emitted from.
if (isValueProvider(provider)) {
emitInstanceCreatedByInjectorEvent(provider.useValue);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Instance created by injector" is a bit confusing here, since we're not creating any instance in the injector. Maybe "dependency satisfied by injector" is a more accurate phrase?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh good point. Lets address this and #48639 (comment) in a followup PR. I'll make a note of it so I don't forget

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still applies :)

@alxhub
Copy link
Copy Markdown
Member

alxhub commented Aug 1, 2023

This PR was merged into the repository by commit c1dee4c.

@alxhub alxhub closed this in ff4d1b4 Aug 1, 2023
alxhub pushed a commit that referenced this pull request Aug 1, 2023
…hProviders` to callback style interfaces (#48639)

walkProviderTree and processInjectorTypesWithProviders both perform some generic traversal logic of the import graph of an input NgModule or Standalone component. Currently, these functions pass around a `providersOut` array that is used to collect providers at each step of the traversal.

This PR converts those functions to accept visitor callbacks instead of the `providersOut` array. This is done to make the traversal logic of these functions reusable, while leaving it up to the visitor to determine the logic that fires for each visited node.

This refactor would allow us to reuse `walkProviderTree` for injector debugging APIs that could support some cool features in DevTools, like tracing the injector resolution path of an injected property on a component instance all the way up to the specific imported module/standalone component.

PR Close #48639
alxhub pushed a commit that referenced this pull request Aug 1, 2023
This commit introduces 3 new APIs.

getDependenciesFromInstantiation:
- Given an injector and a token that was instantiated by that injector, discover all of the things were injected in the instance of that token
- This API is meant to enable recursive inspection of dependencies. Dependencies returned by this API include which injector they were providedIn, which enables the continous use of getDependenciesFromInstantiation to determine the dependencies of dependencies

getInjectorProviders:
- Given an injector, discover all of the providers that were configured in that injector.
- This API returns information on the configured providers of an injector, including the import path that leads to the container that the provider originated from (NgModule or standalone directive). This enables fine grained inspection to determine where a specific provider comes from.

getInjectorParent:
- Given an injector, discover the parent of that injector.
- This function is meant to be used recursively to discover the entire resolution path from a starting injector to the NullInjector.

These APIs were designed to be used together. For example, getInjectorParent can be used to discover the structure of an injector hierarchy. Once that's done, getInjectorProviders can be used to determine the providers of each injector in that hierarchy.

Another example: getDependenciesFromInstantiation can be used to discover the dependencies of a specific injector constructed instance. From there, we can use getInjectorParent to discover the injector resolution path and map each dependency to a path from the starting injector to the injector that it was provided in.

PR Close #48639
alxhub pushed a commit that referenced this pull request Aug 1, 2023
…ng APIs (#48639)

    Creates unit tests for the following APIs

    - setInjectorProfiler
    - getInjectorProviders
    - getInjectorResolutionPath
    - getDependenciesFromInjectable

    Modifies existing tests in

    - packages/examples/core/di/ts/injector_spec.ts
    - packages/core/test/render3/jit/declare_injectable_spec.ts
    - packages/core/test/render3/jit/declare_factory_spec.ts

    because they setup framework injector context manually.

    Exports setInjectorProfilerContext in packages/core/src/core_private_export.ts in order for use
    in the the modified tests above.

PR Close #48639
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 1, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…8639)

Currently, understanding dependency injection in Angular requires a lot of context and has been sited in our surveys as one of the largest points of confusion that our users have with the framework. This commit is the beginning of our approach to make debugging dependency injection in Angular easier.

This commit introduces injector profiler callbacks in parts of the framework to emit injector events. It also introduces a default handler for these events. This default handler parses the stream of events to construct some data structures that will support new injector debug APIs.

We have implemented a similar pattern in the past to minimize overhead. There is also the possiblity of making the internal `setInjectorProfiler` function a public debug API in the future, so that users can implement their own handlers to debug DI events. Lastly DI in Angular maps nicely to a stream of events.

For production applications there is no runtime overhead. For applications in dev mode there is some additional overhead from the default profiler handling injector events and holding debug data in memory.

For production applications, dead code elimination should strip all of the code used by this PR.

PR Close angular#48639
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: devtools detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants