Skip to content
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(Angular): Add URL Parameterization of Transaction Names #5416

Merged
merged 15 commits into from Jul 15, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jul 13, 2022

This PR introduces URL Parameterization to our Angular SDK. With this change, the SDK will update transaction names coming from a URL with a paramterized version of the URL (e.g GET /users/1235/details will be parameterized to GET /users/:userId/details/).

This is done by listening to the ResolveEnd routing event in TraceService. When this event is fired, the Angular router has finished resolving the new URL and found the correct route. Besides the url, the event contains a snapshot of the resolved and soon-to-be activated route. This ActivatedRouteSnapshot instance is the root instance of the activated route. If the resolved route is something other than '' or '/', it will also points to its first child. This instance might again point to its (a possibly existing) child but it also holds its part of the resolved and parameterized route path (URL).
By recursively concatenating the paths, we get a fully parameterized URL.

The main advantage of this approach vs. a previously tried URL<->parameter interpolation approach is that we do not run into the danger of making interpolation mistakes or leaving parts of the route unparameterized. We now simply take what the Angular router has already resolved.

The potential disadvantage is that we rely on the assumption that there is only one child per route snapshot. While testing, I didn't come across a situation where a parent snapshot had more than one child. I believe that this is because the ResolveEnd event contains a snapshot of the newly activated route and not the complete router state. However, if we get reports of incorrect transaction names, we might want to revisit this parameterization approach.

It should be noted that because there is a gap between transaction creation (where we initially set the unparameterized name) and URL parameterization, it is possible that parameterization might happen after an outgoing Http request is made. In that case, the dynamic sampling context will be populated and frozen without the transaction field because at this point the transaction name's source is still url. This means that we have a potential timing problem, similar to other routing instrumentations.
At this point we're not yet sure how often such a timing problem would occur but it seems pretty unlikely for normal usage. For instance, DSC population will happen correctly (with the transaction field) when the first outgoing Http request is fired in the constructor of the component that is associated with the new route. This also means that hooks like ngOnInit which are frequently used for making requests (e.g. via Service calls) are called long after the ResolveEnd routing event.

Additionally, this PR adds a few unit tests to test this new behaviour. However, these tests are really unit tests, meaning they only test our TraceService implementation. We currently simply mock the Angular router and fire the routing events manually. A more "wholesome" approach (e.g. calling router.navigate instead of manually firing events) would be better but for this we'd need to inject an actual Angular Router into TraceService. This is done best with Angular's TestBed feature but it currently doesn't work with our testing setup for the Angular SDK. Changing the setup is out of scope for this PR but we'll revisit this and make the necessary changes to improve the testing setup of our Angular SDK.

ref: #5342

@Lms24 Lms24 self-assigned this Jul 13, 2022
@Lms24 Lms24 added this to the Dynamic Sampling Context milestone Jul 13, 2022
@Lms24 Lms24 marked this pull request as ready for review July 13, 2022 11:00
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
@Lms24

This comment was marked as outdated.

@Lms24 Lms24 requested a review from AbhiPrasad July 14, 2022 10:58
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Generally looks very good to me, since you already addressed Abhi's feedback. I just have a few questions regarding angular routing in general and one little TS improvement.

packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
* @returns a map of params, mapping from a key to an array of values
*/
export function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): ParamMap {
function getParamsOfRouteH(routeSnapshot: ActivatedRouteSnapshot, accParams: ParamMap): ParamMap {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this is a tree and not just a chain of route snapshots. A tree would imply that there are multiple routes active (leaf nodes of the tree), can that be the case? Multi-layouts? Do we consider those as "navigation events" where we want to start transactions?

Copy link
Member Author

Choose a reason for hiding this comment

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

So my best guess is that we're not going to encounter multi-child routes, which is why for the new approach we're essentially treating the route snapshot as a chain instead of a tree. My assumption: The ResolveEnd event holds the router state of the newly resolved (and soon to be activated) route and not the entire router state.
If we get reports of wrong transaction names, we can always revisit this strategy. Although I'd argue that in this case we'd need to rethink how we treat such navigations (i.e. how the transaction name should look like then).

packages/angular/src/tracing.ts Outdated Show resolved Hide resolved
@Lms24 Lms24 requested a review from lforst July 15, 2022 08:04
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Good work dude!

packages/angular/test/tracing.test.ts Show resolved Hide resolved
@Lms24 Lms24 merged commit a87c0c6 into master Jul 15, 2022
@Lms24 Lms24 deleted the lms-urls-angular branch July 15, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants