-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add surface label #1162
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: add surface label #1162
Changes from all commits
a287361
95918bc
7632869
c97d41d
79e5b2c
dc11e54
12d9a46
cb2c524
d16ae7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,3 +14,5 @@ dist | |
| test-results | ||
| build-test-results | ||
| .turbo/ | ||
| .claude/settings.local.json | ||
| .vscode | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import { KEY_APP_SURFACE_LABEL } from '../constants/index.ts'; | ||
|
|
||
| // These attributes are exposed in the public API for users to use in the attributes scrubber | ||
| export const attributes = { | ||
| pageLabel: KEY_APP_SURFACE_LABEL, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| export { attributes } from './attributes.ts'; | ||
| export type { | ||
| AttributeScrubber, | ||
| PathnameDocument, | ||
| TitleDocument, | ||
| URLDocument, | ||
| VisibilityStateDocument, | ||
| } from './types.ts'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,57 @@ | ||
| import type { PageManager, Route } from '../../api-page/index.ts'; | ||
| import type { TitleDocument } from '../../common/index.ts'; | ||
| import { generateUUID } from '../../utils/index.ts'; | ||
| import type { EmbracePageManagerArgs } from './types.ts'; | ||
|
|
||
| export class EmbracePageManager implements PageManager { | ||
| private _currentRoute: Route | null = null; | ||
| private _currentPageId: string | null = null; | ||
| private _pageLabel: string | null = null; | ||
| private readonly _titleDocument: TitleDocument | undefined; | ||
| private readonly _useDocumentTitleAsPageLabel: boolean; | ||
|
|
||
| public constructor({ | ||
| useDocumentTitleAsPageLabel = true, | ||
| titleDocument = window.document, | ||
overbalance marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }: EmbracePageManagerArgs = {}) { | ||
| this._useDocumentTitleAsPageLabel = useDocumentTitleAsPageLabel; | ||
| this._titleDocument = titleDocument; | ||
| } | ||
|
|
||
| public getCurrentPageId = (): string | null => this._currentPageId; | ||
|
|
||
| public getCurrentRoute = () => this._currentRoute; | ||
|
|
||
| public setPageLabel = (label: string): void => { | ||
| this._pageLabel = label; | ||
| }; | ||
|
|
||
| public getPageLabel = (): string | null => { | ||
| return ( | ||
| this._pageLabel || | ||
| (!this._useDocumentTitleAsPageLabel || !this._titleDocument | ||
| ? null | ||
| : this._titleDocument.title) | ||
| ); | ||
| }; | ||
|
|
||
| public setCurrentRoute = (route: Route) => { | ||
| if (!this._currentRoute || this._currentRoute.url !== route.url) { | ||
| this._currentPageId = generateUUID(); | ||
| } | ||
|
|
||
| this._currentRoute = route; | ||
|
|
||
| if (route.label) { | ||
| this._pageLabel = route.label; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the route changes but no label is set, it will reuse the previous label. Should this be cleared out instead?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, good catch |
||
| } else { | ||
| this._pageLabel = null; | ||
| } | ||
| }; | ||
|
|
||
| public clearCurrentRoute = () => { | ||
| this._currentRoute = null; | ||
| this._currentPageId = null; | ||
| this._pageLabel = null; | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| import type { LogRecordProcessor, SdkLogRecord } from '@opentelemetry/sdk-logs'; | ||
| import type { PageManager } from '../../api-page/index.ts'; | ||
| import { KEY_EMB_PAGE_ID, KEY_EMB_PAGE_PATH } from '../../constants/index.ts'; | ||
| import { | ||
| KEY_APP_SURFACE_LABEL, | ||
| KEY_EMB_PAGE_ID, | ||
| KEY_EMB_PAGE_PATH, | ||
| } from '../../constants/index.ts'; | ||
| import type { PageLogRecordProcessorArgs } from './types.ts'; | ||
|
|
||
| export class PageLogRecordProcessor implements LogRecordProcessor { | ||
|
|
@@ -16,22 +20,25 @@ export class PageLogRecordProcessor implements LogRecordProcessor { | |
| } | ||
|
|
||
| public onEmit(logRecord: SdkLogRecord): void { | ||
| // If the log already has page attributes, do not override them | ||
| if ( | ||
| logRecord.attributes[KEY_EMB_PAGE_PATH] || | ||
| logRecord.attributes[KEY_EMB_PAGE_ID] | ||
| !logRecord.attributes[KEY_EMB_PAGE_PATH] || | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If either is missing it overrides both
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yep that makes sense to me |
||
| !logRecord.attributes[KEY_EMB_PAGE_ID] | ||
| ) { | ||
| // If the log already has page attributes, do not override them | ||
| return; | ||
| } | ||
| const currentRoute = this._pageManager.getCurrentRoute(); | ||
|
|
||
| const currentRoute = this._pageManager.getCurrentRoute(); | ||
| if (currentRoute) { | ||
| logRecord.setAttribute(KEY_EMB_PAGE_PATH, currentRoute.path); | ||
| logRecord.setAttribute( | ||
| KEY_EMB_PAGE_ID, | ||
| this._pageManager.getCurrentPageId(), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (currentRoute) { | ||
| logRecord.setAttribute(KEY_EMB_PAGE_PATH, currentRoute.path); | ||
| logRecord.setAttribute( | ||
| KEY_EMB_PAGE_ID, | ||
| this._pageManager.getCurrentPageId(), | ||
| ); | ||
| const appSurfaceLabel = this._pageManager.getPageLabel(); | ||
| if (appSurfaceLabel && !logRecord.attributes[KEY_APP_SURFACE_LABEL]) { | ||
| logRecord.setAttribute(KEY_APP_SURFACE_LABEL, appSurfaceLabel); | ||
| } | ||
| } | ||
|
|
||
|
|
||
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 like the examples given above. Maybe you could add something like:
"OLED Televisions - Electronics - BigStore.com"