-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(Angular): Add URL Parameterization of Transaction Names #5416
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this 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
* @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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work dude!
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 toGET /users/:userId/details/
).This is done by listening to the
ResolveEnd
routing event inTraceService
. 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. ThisActivatedRouteSnapshot
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 stillurl
. 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 likengOnInit
which are frequently used for making requests (e.g. via Service calls) are called long after theResolveEnd
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. callingrouter.navigate
instead of manually firing events) would be better but for this we'd need to inject an actual Angular Router intoTraceService
. This is done best with Angular'sTestBed
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