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

ai_user cookie not present after reenabling the cookie #1585 #1632

Merged
merged 2 commits into from
Aug 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions common/config/rush/npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 22 additions & 4 deletions extensions/applicationinsights-properties-js/src/Context/User.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,16 @@ export class User implements IUserContext {
*/
public accountAcquisitionDate: string;

/**
* A flag indicating whether this represents a new user
*/
public isNewUser = false;

/**
* A flag indicating whether the user cookie has been set
*/
public isUserCookieSet = false;

constructor(config: ITelemetryConfig, core: IAppInsightsCore) {
let _logger = safeGetLogger(core);
let _cookieManager: ICookieMgr = safeGetCookieMgr(core);
Expand All @@ -75,6 +83,8 @@ export class User implements IUserContext {
const params = cookie.split(User.cookieSeparator);
if (params.length > 0) {
_self.id = params[0];
// we already have a cookie
_self.isUserCookieSet = !!_self.id;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we successfully read a value from the cookie then we assume that the cookie was set.

}
}

Expand All @@ -98,7 +108,7 @@ export class User implements IUserContext {
// set it to 365 days from now
// 365 * 24 * 60 * 60 = 31536000
const oneYear = 31536000;
_cookieManager.set(_storageNamePrefix(), cookie, oneYear);
_self.isUserCookieSet = _cookieManager.set(_storageNamePrefix(), cookie, oneYear);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use new response that tells us if the cookie was really set.

}

if (!_self.id) {
Expand Down Expand Up @@ -172,9 +182,12 @@ export class User implements IUserContext {
};

_self.update = (userId?: string) => {
let user_id = userId ? userId : _generateNewId();
let user_cookie = _generateNewCookie(user_id);
_setUserCookie(user_cookie.join(User.cookieSeparator));
// Optimizations to avoid setting and processing the cookie when not needed
if (_self.id !== userId || !_self.isUserCookieSet) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add an extra performance based check so we don't try and set the cookie if

  • The value was already set
  • The new userId is the same as the previous one.

let user_id = userId ? userId : _generateNewId();
let user_cookie = _generateNewCookie(user_id);
_setUserCookie(user_cookie.join(User.cookieSeparator));
}
};
});
}
Expand All @@ -197,6 +210,11 @@ export class User implements IUserContext {
// @DynamicProtoStub -- DO NOT add any code as this will be removed during packaging
}

/**
* Update or create the user cookie if cookies where previously disabled or the new userId does not match the existing value.
* If you pass nothing a new random user id will be created.
* @param userId - Specific either the current (via appInsights.context.user.id) or new id that you want to set
*/
public update(userId?: string) {
// @DynamicProtoStub -- DO NOT add any code as this will be removed during packaging
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,15 @@ export default class PropertiesPlugin extends BaseTelemetryPlugin implements IPr
}
}

if (theContext.user) {
theContext.user.update(theContext.user.id);
let userCtx = theContext.user;
if (userCtx && !userCtx.isUserCookieSet) {
userCtx.update(theContext.user.id);
}

_processTelemetryInternal(event, itemCtx);

if (theContext.user && theContext.user.isNewUser) {
theContext.user.isNewUser = false;
if (userCtx && userCtx.isNewUser) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simple minification improvement which also has a miner perf improvement.

userCtx.isNewUser = false;
const message = new _InternalLogMessage(_InternalMessageId.SendBrowserInfoOnUserInit, ((getNavigator()||{} as any).userAgent||""));
itemCtx.diagLog().logInternalMessage(LoggingSeverity.CRITICAL, message);
}
Expand Down
5 changes: 5 additions & 0 deletions shared/AppInsightsCommon/src/Interfaces/Context/IUser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ export interface IUser {
* A flag indicating whether this represents a new user
*/
isNewUser?: boolean;

/**
* A flag indicating whether the user cookie has been set
*/
isUserCookieSet?: boolean;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding as optional so we don't break anyone who is using this interface

}

export interface IUserContext extends IUser {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ export interface ICookieMgr {
* @param maxAgeSec - [optional] The maximum number of SECONDS that this cookie should survive
* @param domain - [optional] The domain to set for the cookie
* @param path - [optional] Path to set for the cookie, if not supplied will default to "/"
* @returns - True if the cookie was set otherwise false (Because cookie usage is not enabled or available)
*/
set(name: string, value: string, maxAgeSec?: number, domain?: string, path?: string): void;
set(name: string, value: string, maxAgeSec?: number, domain?: string, path?: string): boolean;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While this is a change to the signature of the function, it should be backward compatible as previously it was void.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is also used by internally as well, so this should be compatible with that as well.


/**
* Get the value of the named cookie
Expand All @@ -34,17 +35,19 @@ export interface ICookieMgr {
* Note: Not using "delete" as the name because it's a reserved word which would cause issues on older browsers
* @param name - The name of the cookie
* @param path - [optional] Path to set for the cookie, if not supplied will default to "/"
* @returns - True if the cookie was marked for deletion otherwise false (Because cookie usage is not enabled or available)
*/
del(name: string, path?: string): void;
del(name: string, path?: string): boolean;

/**
* Purge the cookie from the system if cookie support is available, this function ignores the enabled setting of the manager
* so any cookie will be removed.
* Note: Not using "delete" as the name because it's a reserved word which would cause issues on older browsers
* @param name - The name of the cookie
* @param path - [optional] Path to set for the cookie, if not supplied will default to "/"
* @returns - True if the cookie was marked for deletion otherwise false (Because cookie usage is not available)
*/
purge(name: string, path?: string): void;
purge(name: string, path?: string): boolean;
}

/**
Expand Down
13 changes: 12 additions & 1 deletion shared/AppInsightsCore/src/JavaScriptSDK/CookieMgr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export function createCookieMgr(rootConfig?: IConfiguration, logger?: IDiagnosti
_enabled = value !== false;
},
set: (name: string, value: string, maxAgeSec?: number, domain?: string, path?: string) => {
let result = false;
if (_isMgrEnabled(cookieMgr)) {
let values: any = {};
let theValue = strTrim(value || strEmpty);
Expand Down Expand Up @@ -189,7 +190,10 @@ export function createCookieMgr(rootConfig?: IConfiguration, logger?: IDiagnosti

let setCookieFn = cookieMgrConfig.setCookie || _setCookieValue;
setCookieFn(name, _formatCookieValue(theValue, values));
result = true;
}

return result;
MSNev marked this conversation as resolved.
Show resolved Hide resolved
},
get: (name: string): string => {
let value = strEmpty
Expand All @@ -200,12 +204,16 @@ export function createCookieMgr(rootConfig?: IConfiguration, logger?: IDiagnosti
return value;
},
del: (name: string, path?: string) => {
let result = false;
if (_isMgrEnabled(cookieMgr)) {
// Only remove the cookie if the manager and cookie support has not been disabled
cookieMgr.purge(name, path);
result = cookieMgr.purge(name, path);
}

return result;
},
purge: (name: string, path?: string) => {
let result = false;
if (areCookiesSupported(logger)) {
// Setting the expiration date in the past immediately removes the cookie
let values = {
Expand All @@ -220,7 +228,10 @@ export function createCookieMgr(rootConfig?: IConfiguration, logger?: IDiagnosti

let delCookie = cookieMgrConfig.delCookie || _setCookieValue;
delCookie(name, _formatCookieValue(strEmpty, values));
result = true;
}

return result;
}
};

Expand Down