search

vaadin / observability-kit

Other
5 stars 2 forks source link

fix: make navigation span show target route #98

Closed sissbruecker closed 2 years ago

sissbruecker commented 2 years ago

Fixes the AbstractNavigationStateRendererInstrumentation to show the route that navigation was attempted to, instead of the route that the UI has after navigation. The UI route might not have changed if an error occurs or the navigation is postponed.

The root span still shows the route that the UI had after the navigation as it would not be correct to update it if the navigation was postponed.

Fixes #94

caalador commented 2 years ago

Even though the navigation target is now correct the rootSpan gives conflicting information: image It names route as statistics even though the target is accounts and the browser fails on the route accounts and doesn't revert to statistics. image

sissbruecker commented 2 years ago

@caalador Yeah, it's an intentional decision and a compromise. The current logic is better in cases where no navigation happened at all because the navigation was post-poned by a BeforeLeaveObserver, or the exception happened in a BeforeLeaveObserver. There might be other cases where the browser does not get to update the URL due to a navigation error.

caalador commented 2 years ago

I would rather have beforeLeave wrong than this as it doesn't give the correct data and is most likely much more likely to happen. @emarc what is your take on this.