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: allow session tracking across subdomains #38

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

deborahgu
Copy link
Member

In order to gain the ability to track our sessions, funnels, conversions, journeys, etc. across all of our various applications and subdomains, we need to add a couple of settings to RUM and Logs SDK setup.

From Datadog Browser Log Collection and Datadog Log Collection initialization parameters:

  • trackSessionAcrossSubdomains: Preserve the session across subdomains for the same site.
  • usePartitionedCrossSiteSessionCookie: Use a partitioned secure cross-site session cookie. This allows the logs SDK to run when the site is loaded from another one (iframe). Implies useSecureSessionCookie.

Per DD support:

The useCrossSiteSessionCookie parameter specifies the type of cookie the SDK will set, it's not a reference to cross-domain tracking. You would want this to be set to true to track events when the page is rendered as an iframe.

Note: useCrossSiteSessionCookie is marked in the documents as a deprecated parameter which we should replace withusePartitionedCrossSiteSessionCookie.

In order to gain the ability to track our sessions, funnels, conversions, journeys, etc. across all of our various applications and subdomains, we need to add a couple of settings to RUM and Logs SDK setup.

From [Datadog Browser Log Collection](https://docs.datadoghq.com/real_user_monitoring/browser/setup/#initialization-parameters) and [Datadog Log Collection initialization parameters](https://docs.datadoghq.com/logs/log_collection/javascript/#initialization-parameters):

* `trackSessionAcrossSubdomains`:  Preserve the session across subdomains for the same site.
* `usePartitionedCrossSiteSessionCookie`: Use a partitioned secure cross-site session cookie. This allows the logs SDK to run when the site is loaded from another one (iframe). Implies `useSecureSessionCookie`.

Per DD support:

> The `useCrossSiteSessionCookie` parameter specifies the type of cookie the SDK will set, it's not a reference to cross-domain tracking. You would want this to be set to `true` to track events when the page is rendered as an iframe.

Note: `useCrossSiteSessionCookie`  is marked in the documents as a deprecated perimeter which we should replace with`usePartitionedCrossSiteSessionCookie`.
reminding deployers to update the package version number.
README.md Outdated
@@ -15,6 +15,10 @@ You can also add your own custom metrics as an additional argument, or see the c

General Logging service interface is defined in [frontend-platform](https://openedx.github.io/frontend-platform/module-Logging.LoggingService.html).

## Deployment

When you make changes, [draft a new release](https://github.com/edx/frontend-logging/releases) (and, if necessary, change the imported version number in importing packages).
Copy link
Member

Choose a reason for hiding this comment

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

[clarification] semantic-release should be creating GitHub releases automatically after merging a PR. For example, the existing releases are created by the edx-semantic-release user.

[clarification x2] Is if necessary, change the imported version number in importing packages referring to bumping versions of frontend-logging in edx-internal?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. ohhh thanks, that's not happening in every repo we import as a package so I didn't realize
  2. Yeah, but if I'm taking out (1) I might as well take out the bumping version reference, don't you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree with (2); I think the version bumping is also largely implied as a consumer of this package (for the most part?). I feel if we keep a "Deployment" section, it might largely serve to document that this repository uses Semantic Release to automate the release process (creating Git tags, creating GitHub releases, and publishing to NPM).

@@ -60,6 +60,8 @@ class DatadogLoggingService extends NewRelicLoggingService {
sessionSampleRate: parseInt(process.env.DATADOG_SESSION_SAMPLE_RATE || 0, 10),
sessionReplaySampleRate: parseInt(process.env.DATADOG_SESSION_REPLAY_SAMPLE_RATE || 0, 10),
trackUserInteractions: true,
trackSessionAcrossSubdomains: true,
Copy link
Member

Choose a reason for hiding this comment

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

Based on the documentation:

Options that must have matching configuration when you are using the Logs Browser SDK:

Options that must have a matching configuration when using the RUM SDK:

Given this, I wonder if it might make sense to abstract common options across datadogRum.init and datadogLogs.init so it doesn't necessitate manually supplying the same values in both; might help to guarantee they are always matching, for example. Something like:

const datadogVersion = process.env.DATADOG_VERSION || process.env.APP_VERSION || '1.0.0';
const commonInitOptions = {
  clientToken: process.env.DATADOG_CLIENT_TOKEN,
  site: process.env.DATADOG_SITE || '',
  env: process.env.DATADOG_ENV || '',
  service: process.env.DATADOG_SERVICE || '',
  version: datadogVersion,
  trackSessionAcrossSubdomains: true,
  usePartitionedCrossSiteSessionCookie: true,
};
datadogRum.init({
  ...commonInitOptions,
  beforeSend: this.beforeSend,
  sessionSampleRate: parseInt(process.env.DATADOG_SESSION_SAMPLE_RATE || 0, 10),
  sessionReplaySampleRate: parseInt(process.env.DATADOG_SESSION_REPLAY_SAMPLE_RATE || 0, 10),
  trackUserInteractions: true,
  trackResources: true,
  trackLongTasks: true,
  defaultPrivacyLevel: process.env.DATADOG_PRIVACY_LEVEL || 'mask-user-input',
});
datadogLogs.init({
  ...commonInitOptions,
  forwardErrorsToLogs: true,
  sessionSampleRate: parseInt(process.env.DATADOG_LOGS_SESSION_SAMPLE_RATE || 0, 10),
});

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this, implementing.

Per code review suggestions,

* pulling common options out so we don't have repetition (that can get out of sync)
*  simplifying the documentation, since I hadn't realized we used semantic releases
@deborahgu deborahgu merged commit 1bf9c15 into master Aug 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants