Skip to content

Commit

Permalink
Remove baseLogger and scopeOverride options from Loader/Container (#1…
Browse files Browse the repository at this point in the history
…4216)

This change removes the features added in #13779 and #14114, as we've
determined they are not actually needed.
  • Loading branch information
ChumpChief authored Feb 16, 2023
1 parent f9e907b commit 52e5f1f
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 78 deletions.
4 changes: 0 additions & 4 deletions api-report/container-loader.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,25 +127,21 @@ export interface ICodeDetailsLoader extends Partial<IProvideFluidCodeDetailsComp

// @public (undocumented)
export interface IContainerConfig {
baseLogger?: ITelemetryBaseLogger;
// (undocumented)
canReconnect?: boolean;
clientDetailsOverride?: IClientDetails;
// (undocumented)
resolvedUrl?: IFluidResolvedUrl;
scopeOverride?: FluidObject;
serializedContainerState?: IPendingContainerState;
}

// @public (undocumented)
export interface IContainerLoadOptions {
baseLogger?: ITelemetryBaseLogger;
canReconnect?: boolean;
clientDetailsOverride?: IClientDetails;
loadMode?: IContainerLoadMode;
// (undocumented)
resolvedUrl: IFluidResolvedUrl;
scopeOverride?: FluidObject;
version: string | undefined;
}

Expand Down
6 changes: 0 additions & 6 deletions packages/common/container-definitions/src/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -544,12 +544,6 @@ export enum LoaderHeader {
* otherwise, version sha to load snapshot
*/
version = "version",

// TODO #AB3350: This is a breaking change; it will be enabled in the "next" branch
// baseLogger = "fluid-base-logger",

// TODO #AB???: This is a breaking change; it will be enabled in the "next" branch
// scopeOverride = "fluid-scope-override",
}

export interface IContainerLoadMode {
Expand Down
90 changes: 31 additions & 59 deletions packages/loader/container-loader/src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@
import merge from "lodash/merge";
import { v4 as uuid } from "uuid";
import {
ITelemetryBaseLogger,
ITelemetryLogger,
ITelemetryProperties,
TelemetryEventCategory,
} from "@fluidframework/common-definitions";
import { assert, performance, unreachableCase } from "@fluidframework/common-utils";
import { IRequest, IResponse, IFluidRouter, FluidObject } from "@fluidframework/core-interfaces";
import { IRequest, IResponse, IFluidRouter } from "@fluidframework/core-interfaces";
import {
IAudience,
IConnectionDetails,
Expand Down Expand Up @@ -124,15 +123,6 @@ export interface IContainerLoadOptions {
* Loads the Container in paused state if true, unpaused otherwise.
*/
loadMode?: IContainerLoadMode;
/**
* A logger that the container will use for logging operations. If not provided, the container will
* use the loader's logger, `Loader.services.subLogger`.
*/
baseLogger?: ITelemetryBaseLogger;
/**
* A scope object to replace the one provided on the Loader.
*/
scopeOverride?: FluidObject;
}

export interface IContainerConfig {
Expand All @@ -146,15 +136,6 @@ export interface IContainerConfig {
* Serialized state from a previous instance of this container
*/
serializedContainerState?: IPendingContainerState;
/**
* A logger that the container will use for logging operations. If not provided, the container will
* use the loader's logger, `Loader.services.subLogger`.
*/
baseLogger?: ITelemetryBaseLogger;
/**
* A scope object to replace the one provided on the Loader.
*/
scopeOverride?: FluidObject;
}

/**
Expand Down Expand Up @@ -310,8 +291,6 @@ export class Container
resolvedUrl: loadOptions.resolvedUrl,
canReconnect: loadOptions.canReconnect,
serializedContainerState: pendingLocalState,
baseLogger: loadOptions.baseLogger,
scopeOverride: loadOptions.scopeOverride,
},
protocolHandlerBuilder,
);
Expand Down Expand Up @@ -464,7 +443,6 @@ export class Container
}

private readonly clientDetailsOverride: IClientDetails | undefined;
private readonly _scopeOverride: FluidObject | undefined;
private readonly _deltaManager: DeltaManager<ConnectionManager>;
private service: IDocumentService | undefined;

Expand Down Expand Up @@ -615,7 +593,7 @@ export class Container
}
public readonly options: ILoaderOptions;
private get scope() {
return this._scopeOverride ?? this.loader.services.scope;
return this.loader.services.scope;
}
private get codeLoader() {
return this.loader.services.codeLoader;
Expand All @@ -637,7 +615,6 @@ export class Container
});

this.clientDetailsOverride = config.clientDetailsOverride;
this._scopeOverride = config.scopeOverride;
this._resolvedUrl = config.resolvedUrl;
if (config.canReconnect !== undefined) {
this._canReconnect = config.canReconnect;
Expand All @@ -651,41 +628,36 @@ export class Container
}`;
// Need to use the property getter for docId because for detached flow we don't have the docId initially.
// We assign the id later so property getter is used.
this.subLogger = ChildLogger.create(
// If a baseLogger was provided, use it; otherwise use the loader's logger.
config.baseLogger ?? loader.services.subLogger,
undefined,
{
all: {
clientType, // Differentiating summarizer container from main container
containerId: uuid(),
docId: () => this._resolvedUrl?.id ?? undefined,
containerAttachState: () => this._attachState,
containerLifecycleState: () => this._lifecycleState,
containerConnectionState: () => ConnectionState[this.connectionState],
serializedContainer: config.serializedContainerState !== undefined,
},
// we need to be judicious with our logging here to avoid generating too much data
// all data logged here should be broadly applicable, and not specific to a
// specific error or class of errors
error: {
// load information to associate errors with the specific load point
dmInitialSeqNumber: () => this._deltaManager?.initialSequenceNumber,
dmLastProcessedSeqNumber: () => this._deltaManager?.lastSequenceNumber,
dmLastKnownSeqNumber: () => this._deltaManager?.lastKnownSeqNumber,
containerLoadedFromVersionId: () => this.loadedFromVersion?.id,
containerLoadedFromVersionDate: () => this.loadedFromVersion?.date,
// message information to associate errors with the specific execution state
// dmLastMsqSeqNumber: if present, same as dmLastProcessedSeqNumber
dmLastMsqSeqNumber: () => this.deltaManager?.lastMessage?.sequenceNumber,
dmLastMsqSeqTimestamp: () => this.deltaManager?.lastMessage?.timestamp,
dmLastMsqSeqClientId: () => this.deltaManager?.lastMessage?.clientId,
dmLastMsgClientSeq: () => this.deltaManager?.lastMessage?.clientSequenceNumber,
connectionStateDuration: () =>
performance.now() - this.connectionTransitionTimes[this.connectionState],
},
this.subLogger = ChildLogger.create(loader.services.subLogger, undefined, {
all: {
clientType, // Differentiating summarizer container from main container
containerId: uuid(),
docId: () => this._resolvedUrl?.id ?? undefined,
containerAttachState: () => this._attachState,
containerLifecycleState: () => this._lifecycleState,
containerConnectionState: () => ConnectionState[this.connectionState],
serializedContainer: config.serializedContainerState !== undefined,
},
);
// we need to be judicious with our logging here to avoid generating too much data
// all data logged here should be broadly applicable, and not specific to a
// specific error or class of errors
error: {
// load information to associate errors with the specific load point
dmInitialSeqNumber: () => this._deltaManager?.initialSequenceNumber,
dmLastProcessedSeqNumber: () => this._deltaManager?.lastSequenceNumber,
dmLastKnownSeqNumber: () => this._deltaManager?.lastKnownSeqNumber,
containerLoadedFromVersionId: () => this.loadedFromVersion?.id,
containerLoadedFromVersionDate: () => this.loadedFromVersion?.date,
// message information to associate errors with the specific execution state
// dmLastMsqSeqNumber: if present, same as dmLastProcessedSeqNumber
dmLastMsqSeqNumber: () => this.deltaManager?.lastMessage?.sequenceNumber,
dmLastMsqSeqTimestamp: () => this.deltaManager?.lastMessage?.timestamp,
dmLastMsqSeqClientId: () => this.deltaManager?.lastMessage?.clientId,
dmLastMsgClientSeq: () => this.deltaManager?.lastMessage?.clientSequenceNumber,
connectionStateDuration: () =>
performance.now() - this.connectionTransitionTimes[this.connectionState],
},
});

// Prefix all events in this file with container-loader
this.mc = loggerToMonitoringContext(ChildLogger.create(this.subLogger, "Container"));
Expand Down
10 changes: 1 addition & 9 deletions packages/loader/container-loader/src/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -460,13 +460,7 @@ export class Loader implements IHostLoader {
}

private canCacheForRequest(headers: IRequestHeader): boolean {
return (
this.cachingEnabled &&
headers[LoaderHeader.cache] !== false &&
// LoaderHeader.baseLogger and LoaderHeader.scopeOverride must create a new Container to apply the overrides.
headers["fluid-base-logger"] === undefined &&
headers["fluid-scope-override"] === undefined
);
return this.cachingEnabled && headers[LoaderHeader.cache] !== false;
}

private parseHeader(parsed: IParsedUrl, request: IRequest) {
Expand Down Expand Up @@ -504,8 +498,6 @@ export class Loader implements IHostLoader {
resolvedUrl: resolved,
version: request.headers?.[LoaderHeader.version] ?? undefined,
loadMode: request.headers?.[LoaderHeader.loadMode],
baseLogger: request.headers?.["fluid-base-logger"],
scopeOverride: request.headers?.["fluid-scope-override"],
},
pendingLocalState,
this.protocolHandlerBuilder,
Expand Down

0 comments on commit 52e5f1f

Please sign in to comment.