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: add _experimental_longtaskNoStartSession flag #899

Merged
merged 5 commits into from
Dec 9, 2024
Merged

Conversation

Joozty
Copy link
Member

@Joozty Joozty commented Nov 28, 2024

Description

Added _experimental_longtaskNoStartSession. When set to true, no new session will be spawned after the previous one expires when long task occurs. If both _experimental_longtaskNoStartSession and _experimental_allSpansExtendSession are set to true, _experimental_longtaskNoStartSession takes precedence.

List Github issue(s) which this PR fixes.

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

@Joozty Joozty self-assigned this Nov 28, 2024
@Joozty Joozty changed the title feat: do not spawn session from long task in hidden state feat: add _experimental_longtaskNoStartSession flag Nov 29, 2024
@Joozty Joozty changed the title feat: add _experimental_longtaskNoStartSession flag feat: add _experimental_longtaskNoStartSession flag Nov 29, 2024
@@ -52,6 +58,16 @@ export class SplunkLongTaskInstrumentation extends InstrumentationBase {
init(): void {}

private _createSpanFromEntry(entry: PerformanceEntry) {
if (
!!this.initOptions._experimental_longtaskNoStartSession &&
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core of the PR... if there is no session we do not spawn new one when this flag is set to true

otlp?: boolean
}

export interface SplunkOtelWebConfig {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to separate file to avoid circular dependency

@@ -633,6 +515,14 @@ export const SplunkRum: SplunkOtelWebType = {
_experimental_getSessionId() {
return this.getSessionId()
},

_internalOnExternalSpanCreated() {
Copy link
Member Author

Choose a reason for hiding this comment

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

this gets called from session recorder - when span is created we extend expiration (inactivity) time of session

export const SESSION_DURATION_SECONDS = 4 * 60 * 60 // 4 hours
export const SESSION_DURATION_MS = SESSION_DURATION_SECONDS * 1000
export const SESSION_INACTIVITY_TIMEOUT_MS = 15 * 60 * 1000 // 15 minutes
export const SESSION_STORAGE_KEY = '_splunk_rum_sid'
Copy link
Member Author

Choose a reason for hiding this comment

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

same key is used for cookies and localstorage

cookieStore._set(value)
},

_set: throttle((value: string) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Writing to cookies/localstorage is throttled. We write directly on load, when session is created and when page gets hidden.

* limitations under the License.
*
*/
export function throttle<T extends (...args: unknown[]) => any>(func: T, limit: number) {
Copy link
Member Author

Choose a reason for hiding this comment

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

throttle function with visibilitychange flush feature

// safety valve
return
}

const cookieValue = encodeURIComponent(JSON.stringify(sessionState))
const domain = cookieDomain ? `domain=${cookieDomain};` : ''
let cookie = COOKIE_NAME + '=' + cookieValue + '; path=/;' + domain + 'max-age=' + InactivityTimeoutSeconds
let cookie = SESSION_STORAGE_KEY + '=' + cookieValue + '; path=/;' + domain + 'max-age=' + SESSION_DURATION_SECONDS
Copy link
Member Author

Choose a reason for hiding this comment

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

max-age is set to session duration and expiresAt is in cookie state

Copy link

Choose a reason for hiding this comment

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

I would prefer a template string here.

@Joozty Joozty marked this pull request as ready for review December 6, 2024 10:55
@Joozty Joozty requested review from a team as code owners December 6, 2024 10:55
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.

Nice job 👍

potty
potty previously requested changes Dec 6, 2024
packages/web/src/session/utils.ts Outdated Show resolved Hide resolved
Copy link

@millcek millcek left a comment

Choose a reason for hiding this comment

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

Nice job 🤞

@Joozty Joozty requested a review from potty December 9, 2024 10:57
@amertak amertak dismissed potty’s stale review December 9, 2024 11:44

Dismissed by changes

@Joozty Joozty merged commit 21d836c into main Dec 9, 2024
6 checks passed
@Joozty Joozty deleted the fix/longtasks branch December 9, 2024 11:46
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 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.

3 participants