Conversation
Chrome DevTools Protocol Tracing (Script: 122.04ms, Heap: 14.20MB)
Lighthouse (Script Eval: 132.94ms)
Platform Tests (vite-7 es2015 gzip: 66.48KB)vite-6 Platform Tests
vite-7 Platform Tests
webpack-5 Platform Tests
|
a6709e4 to
95918bc
Compare
| ); | ||
| } | ||
|
|
||
| const appSurfaceLabel = this._pageManager.getAppSurfaceLabel(); |
There was a problem hiding this comment.
similar to the check above should we avoid doing this if KEY_APP_SURFACE_LABEL is already set?
| span.attributes[KEY_EMB_PAGE_ID] = currentPageId; | ||
| } | ||
|
|
||
| const appSurfaceLabel = this._pageManager.getAppSurfaceLabel(); |
|
|
||
| public getCurrentRoute = () => this._currentRoute; | ||
|
|
||
| public setAppSurfaceLabel = (label: string): void => { |
There was a problem hiding this comment.
should label instead be included as part of Route and set with setCurrentRoute? Could we end up with inconsistencies with this being set separately?
There was a problem hiding this comment.
In that case you'll be forced to pass something as route.path where you might not have it. I think I'd rather keep them independent right now. I can add it as a optional route value so users can set that up when they setCurrentRoute but I'd still keep the label setter. I'm also going to change the API to be:
setRouteLabel and getRouteLabel to be consistent
| span.attributes[KEY_EMB_PAGE_ID] = currentPageId; | ||
| } | ||
|
|
||
| const appSurfaceLabel = this._pageManager.getPageLabel(); |
There was a problem hiding this comment.
one more on this one and packages/web-sdk/src/processors/PageLogRecordProcessor/PageLogRecordProcessor.ts, feel free to ignore if it's a situation that can't really happen:
should the early exit above just wrap the above block instead? e.g.:
// If the span already has page attributes, do not override them
if (
!(span.attributes[KEY_EMB_PAGE_PATH] &&
span.attributes[KEY_EMB_PAGE_ID])
) {
const currentRoute = this._pageManager.getCurrentRoute();
const currentPageId = this._pageManager.getCurrentPageId();
if (currentRoute && currentPageId) {
span.attributes[KEY_EMB_PAGE_PATH] = currentRoute.path;
span.attributes[KEY_EMB_PAGE_ID] = currentPageId;
}
}
just wondering if there would ever be a case where those attributes are set but we'd still want to continue with the surface label logic
There was a problem hiding this comment.
No, I missed that. It made sense before but not after my changes, updating
| path: string; | ||
| // This is the URL of the route after replacing the URL params. i.e. /products/123 | ||
| url: string; | ||
| // Optional label for the route, used as app.surface.label |
There was a problem hiding this comment.
I like the examples given above. Maybe you could add something like:
"OLED Televisions - Electronics - BigStore.com"
| if ( | ||
| logRecord.attributes[KEY_EMB_PAGE_PATH] || | ||
| logRecord.attributes[KEY_EMB_PAGE_ID] | ||
| !logRecord.attributes[KEY_EMB_PAGE_PATH] || |
There was a problem hiding this comment.
I think the logic changed here inadvertently, before it was skipping the setting if either were set, now it could override if one is set
There was a problem hiding this comment.
If either is missing it overrides both
There was a problem hiding this comment.
Yeah, if one is missing I override both so make sure they are using the same route. This shouldn't happen in theory, we always set them at the same time but just to be safe. That's why before if either one was present we don't change it.
There was a problem hiding this comment.
ah yep that makes sense to me
| this._currentRoute = route; | ||
|
|
||
| if (route.label) { | ||
| this._pageLabel = route.label; |
There was a problem hiding this comment.
If the route changes but no label is set, it will reuse the previous label. Should this be cleared out instead?
There was a problem hiding this comment.
Yep, good catch
| if ( | ||
| logRecord.attributes[KEY_EMB_PAGE_PATH] || | ||
| logRecord.attributes[KEY_EMB_PAGE_ID] | ||
| !logRecord.attributes[KEY_EMB_PAGE_PATH] || |
There was a problem hiding this comment.
If either is missing it overrides both
…ub.com:embrace-io/embrace-web-sdk into joaquin-diaz/feat/EMBR-10673-add-surface-label
| if ( | ||
| logRecord.attributes[KEY_EMB_PAGE_PATH] || | ||
| logRecord.attributes[KEY_EMB_PAGE_ID] | ||
| !logRecord.attributes[KEY_EMB_PAGE_PATH] || |
There was a problem hiding this comment.
ah yep that makes sense to me
What problem is this solving?
Allows users to group telemetry by document title until they provide a better default or they use the (future) URL regex to label parsing in the dash
Short description of changes
How has this been tested?
Unit tests, e2e tests, local run