-
Notifications
You must be signed in to change notification settings - Fork 47
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: Persist logging flag in session #1264
feat: Persist logging flag in session #1264
Conversation
This will ensure the logging mode is consistent across the session and stops collecting logging events after session expiration/inactivity.
Asset Size Report
Merging this pull request will result in the following asset size changes:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## auto-logging #1264 +/- ##
===============================================
Coverage ? 88.90%
===============================================
Files ? 170
Lines ? 7418
Branches ? 1501
===============================================
Hits ? 6595
Misses ? 700
Partials ? 123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Last ran on |
case FEATURE_NAMES.logging: | ||
return !!session |
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.
While this makes sense and I can see why you added this bit, product decided that the logging feature should still import and run if session is disabled.
this.syncWithSessionManager({ | ||
loggingMode: this.loggingMode | ||
}) |
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 think we need to double check that this wont cause issues. One of the two cases that could trigger this line is that session tracking is disabled, meaning there is no session object. I fear calling this method could cause errors when that is the case
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.
Should probably only try to sync with the session manager if we know it exists for sure. Either handled here or in the base class method
if (session.isNew || !this.isSessionTrackingEnabled) { | ||
this.loggingMode = loggingMode | ||
|
||
if (this.isSessionTrackingEnabled) { |
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.
this if statement is redundant now (since syncWith.. does the same check), but nbd
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.
yeah, i wasn't sure if it's worth blocking the extra call so added it. Sounds like probably not worth it (?) I can avoid doing that in the future.
This is another part of the work for the auto-logging feature. The auto-logging mode should stay consistent within a given session and will auto-disable upon session expiration after 4 hours or session inactivity after 30 minutes.
Overview
The auto-logging mode should stay consistent within a given session and will auto-disable upon session expiration after 4 hours or session inactivity after 30 minutes.
If cookies are not enabled, then the logging mode for each page will be set independently based on the rum response.
Related Issue(s)
https://new-relic.atlassian.net/browse/NR-341932
Testing
Added/updated relevant tests for logging with regards to session