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

fix(node): Ensure autoSessionTracking is enabled by default #12790

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Jul 8, 2024

I noticed that we were not actually enabling auto session tracking correctly in node, because we did not check on the correct options object 😬

@mydea mydea requested review from lforst, Lms24 and andreiborza July 8, 2024 08:00
@mydea mydea self-assigned this Jul 8, 2024
getDefaultIntegrationsImpl: (options: Options) => Integration[],
): NodeClient {
const clientOptions = getClientOptions(options, getDefaultIntegrationsImpl);
const options = getClientOptions(_options, getDefaultIntegrationsImpl);
Copy link
Member Author

Choose a reason for hiding this comment

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

You don't really see this here, but we've been checking properties further below in init based off options, not clientOptions, leading to us not using the defaults defined in getClientOptions. IMHO it is confusing to have to options objects and check everything on clientOptions (which is also why this even happened in the first place...) so I changed this to just call this options and use it everywhere (we never really need to access the unprocessed options again afterwards anyhow...)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Good catch! (was this reported in an issue?)

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

I wonder if we can track this on looker somehow

@mydea
Copy link
Member Author

mydea commented Jul 8, 2024

Good catch! (was this reported in an issue?)

Nope, just noticed this while I was preparing the walkthrough for the Node SDK 😅

@mydea
Copy link
Member Author

mydea commented Jul 9, 2024

Note: I also updated the node integration tests to ignore session & sessions envelopes by default, instead of manually ignoring these everywhere. Now, instead you need to unignore them if you want to specifically test them.

@mydea mydea force-pushed the fn/fix-session-tracking branch from 07b1a96 to 1da5e09 Compare July 9, 2024 08:59
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.

3 participants