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: attach browser.instance.id and browser.instance.visibility_state to spans #878

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

Joozty
Copy link
Member

@Joozty Joozty commented Nov 1, 2024

Description

Each span now includes browser.instance.id, a unique ID shared across all same-origin frames/contexts within the browser tab. This ID persists across tab reloads until the tab is closed or a different origin is loaded. Additionally, browser.instance.visibility_state is added to each span to indicate the tab's visibility state (hidden/visible) at the time the span was created.

Type of change

Delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How has this been tested?

Delete options that are not relevant.

  • Manual testing
  • Added integration tests

@Joozty Joozty self-assigned this Nov 1, 2024
@Joozty Joozty requested review from a team as code owners November 1, 2024 09:34
Copy link

github-actions bot commented Nov 1, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Each span now includes `browser.instance.id`, a unique ID shared across all same-origin frames/contexts within the browser tab. This ID persists across tab reloads until the tab is closed or a different origin is loaded. Additionally, `browser.instance.visibility_state` is added to each span to indicate the tab's visibility state (hidden/visible) at the time the span was created.
@Joozty
Copy link
Member Author

Joozty commented Nov 1, 2024

I have read the CLA Document and I hereby sign the CLA

srv-gh-o11y-gdi-cla added a commit to splunk/cla-agreement that referenced this pull request Nov 1, 2024
export const safelyGetSessionStorage = (key: string): string | null => {
let value = null;
try {
value = window.sessionStorage.getItem(key);
Copy link
Contributor

@jtmalinowski jtmalinowski Nov 4, 2024

Choose a reason for hiding this comment

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

okay on the topic of session storage I don't have any strong feelings either way and it seems alright but following MDN's docs:

survives over page reloads and restores.

Page reloads I think we can accept because they'll come up in spans. Restores seem like a rare case. So I think this is okayish?

Duplicating a tab copies the tab's sessionStorage into the new tab.

Not sure about this one but it also seems rare enough?

I think we can just deploy this and check if it's good enough in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

survives over page reloads and restores

I did it intentionally that way. It is representing the browser tab rather than just the current window. I mean it can be adjusted if needed.

Duplicating a tab copies the tab's sessionStorage into the new tab.

I guess it is the same as the "restore" case. In the Cisco agent, we cover these use cases, but it requires a bigger refactor as the initialization would need to be asynchronous, and we would need to postpone span sending until the ID is assigned. It can be built on top of this as it is just an additional improvement.

safelySetSessionStorage(BROWSER_INSTANCE_ID_KEY, browserInstanceId);
}

this._id = browserInstanceId;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we didn't manage to persist it then wouldn't it be better to not attach it to spans? Because now let's say someone blocks session storage, and we see their session, it'll have a different browser.instance.id for each scriptInstance, tricking you into believing that these are separate windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Please check 861bed8

@@ -47,6 +47,7 @@ export class SplunkSpanAttributesProcessor implements SpanProcessor {
span.setAttribute('location.href', location.href);
span.setAttributes(this._globalAttributes);
span.setAttribute('splunk.rumSessionId', getRumSessionId());
span.setAttribute('browser.instance.visibility_state', document.visibilityState);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Member Author

Choose a reason for hiding this comment

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

We can now filter how many sessions consist of only long tasks in a hidden state.

Copy link
Contributor

@potty potty left a comment

Choose a reason for hiding this comment

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

👍

@Joozty Joozty merged commit 2a58d98 into main Nov 6, 2024
6 checks passed
@Joozty Joozty deleted the feat/tab-id branch November 6, 2024 09:32
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants