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.

Sign up for GitHub

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

Jump to bottom

[main] [Possible Break] Removed Tags[] from ITelemetryItem as this was breaking later versions of TypeScript by using the intersection type instead of union type for tags property #2258 #2269

Merged
merged 10 commits into from
Feb 12, 2024
Merged

[main] [Possible Break] Removed Tags[] from ITelemetryItem as this was breaking later versions of TypeScript by using the intersection type instead of union type for tags property #2258 #2269

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3facdbc
change tags type
siyuniu-ms Feb 9, 2024
f2d94e9
Update TelemetryContext.ts
siyuniu-ms Feb 9, 2024
bd9c103
Update TelemetryContext.ts
siyuniu-ms Feb 9, 2024
3f188c6
Merge branch 'main' into siyu/tagfix
siyuniu-ms Feb 9, 2024
db44661
Update TelemetryContext.ts
siyuniu-ms Feb 12, 2024
133cdf1
Merge branch 'siyu/tagfix' of https://github.com/microsoft/Applicatio…
siyuniu-ms Feb 12, 2024
2803f3b
remove tags[]
siyuniu-ms Feb 12, 2024
5558534
add notes
siyuniu-ms Feb 12, 2024
75a5b1e
fix
siyuniu-ms Feb 12, 2024
4b310ea
Merge branch 'main' into siyu/tagfix
siyuniu-ms Feb 12, 2024
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
38 changes: 24 additions & 14 deletions extensions/applicationinsights-properties-js/src/TelemetryContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
IUserContext, IWeb, PageView, dataSanitizeString
} from "@microsoft/applicationinsights-common";
import {
IAppInsightsCore, IDistributedTraceContext, IProcessTelemetryContext, ITelemetryItem, IUnloadHookContainer, _InternalLogMessage,
IAppInsightsCore, IDistributedTraceContext, IProcessTelemetryContext, ITelemetryItem, IUnloadHookContainer, Tags, _InternalLogMessage,
getSetValue, hasWindow, isNullOrUndefined, isString, objKeys, setValue
} from "@microsoft/applicationinsights-core-js";
import { Application } from "./Context/Application";
Expand All @@ -35,6 +35,17 @@ function _nullResult(): string {
return null;
}

function setTageValue(tags: Tags | Tags[], field: string, value: string): void {
// Function body
if (Array.isArray(tags)) {
siyuniu-ms marked this conversation as resolved.
Show resolved Hide resolved
tags.forEach(tag => setValue(tag, field, value, isString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't look correct... Let me try and repo locally to see the errors.

Copy link
Collaborator

@MSNev MSNev Feb 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, looking at the usage a bit deeper and the returned error message, I think we do the following

  • Remove the Tags[] completely so it's just defined as tags?: Tags; as during the serialization process in Serializer.ts we process this field with the _serializeStringMap and if that encounters an "error" it will actually report it as an error (which is what would happen if they do actually pass an incomplete array).
  • Add some release notes about this possible breaking change, at the very least lets call this out in the "name" of this PR, so if someone looks at the history it will be obvious
  • Leave the current "default" initializations as [] as it "seems" happy about that and the serialization is working with it -- even though technically they should be objects.

As this change will only affect npm users and the correct "typing" is really Tags anyway along with the next release being 4.1.0 its a good time to break anyone how might be trying to pass an array (which they shouldn't).

I've already checked the 1DS code with this change and it complies as well.

);
} else {
setValue(tags, field, value, isString)
}
}


export class TelemetryContext implements IPropTelemetryContext {

public application: IApplication; // The object describing a component tracked by this object - legacy
Expand Down Expand Up @@ -106,8 +117,8 @@ export class TelemetryContext implements IPropTelemetryContext {
if (application) {
// evt.ext.app
let tags = getSetValue(evt, strTags);
setValue(tags, CtxTagKeys.applicationVersion, application.ver, isString);
setValue(tags, CtxTagKeys.applicationBuild, application.build, isString)
setTageValue(tags, CtxTagKeys.applicationVersion, application.ver);
setTageValue(tags, CtxTagKeys.applicationBuild, application.build);
}
};

Expand All @@ -127,24 +138,24 @@ export class TelemetryContext implements IPropTelemetryContext {
let internal = _self.internal;
if (internal) {
let tags = getSetValue(evt, strTags);

setValue(tags, CtxTagKeys.internalAgentVersion, internal.agentVersion, isString); // not mapped in CS 4.0
setValue(tags, CtxTagKeys.internalSdkVersion, dataSanitizeString(logger, internal.sdkVersion, 64), isString);

setTageValue(tags, CtxTagKeys.internalAgentVersion, internal.agentVersion);
setTageValue(tags, CtxTagKeys.internalSdkVersion, dataSanitizeString(logger, internal.sdkVersion, 64));
if (evt.baseType === _InternalLogMessage.dataType || evt.baseType === PageView.dataType) {
setValue(tags, CtxTagKeys.internalSnippet, internal.snippetVer, isString);
setValue(tags, CtxTagKeys.internalSdkSrc, internal.sdkSrc, isString);
setTageValue(tags, CtxTagKeys.internalSnippet, internal.snippetVer);
setTageValue(tags, CtxTagKeys.internalSdkSrc, internal.sdkSrc);
}
}
};

_self.applyLocationContext = (evt: ITelemetryItem, itemCtx?: IProcessTelemetryContext) => {
let location = this.location;
if (location) {
setValue(getSetValue(evt, strTags, []), CtxTagKeys.locationIp, location.ip, isString);
let tags = getSetValue(evt, strTags, []);
setTageValue(tags, CtxTagKeys.locationIp, location.ip);
}
};



_self.applyOperationContext = (evt: ITelemetryItem, itemCtx?: IProcessTelemetryContext) => {
let telemetryTrace = _self.telemetryTrace;
if (telemetryTrace) {
Expand All @@ -166,10 +177,8 @@ export class TelemetryContext implements IPropTelemetryContext {
let user = _self.user;
if (user) {
let tags = getSetValue(evt, strTags, []);

// stays in tags
setValue(tags, CtxTagKeys.userAccountId, user.accountId, isString);

setTageValue(tags, CtxTagKeys.userAccountId, user.accountId);
// CS 4.0
let extUser = getSetValue(getSetValue(evt, strExt), Extensions.UserExt);
setValue(extUser, "id", user.id, isString);
Expand All @@ -191,6 +200,7 @@ export class TelemetryContext implements IPropTelemetryContext {
});
}


public applySessionContext(evt: ITelemetryItem, itemCtx?: IProcessTelemetryContext) {
// @DynamicProtoStub -- DO NOT add any code as this will be removed during packaging
}
Expand Down
2 changes: 1 addition & 1 deletion shared/AppInsightsCore/src/JavaScriptSDK.Interfaces/ITelemetryItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export interface ITelemetryItem {
/**
* System context property extensions that are not global (not in ctx)
*/
tags?: Tags & Tags[]; // Tags[] will be deprecated.
tags?: Tags | Tags[]; // Tags[] will be deprecated.

/**
* Custom data
Expand Down
Loading