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

Only clear mod component debug logs if logValues is enabled #9225

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Only clear mod component debug logs if logValues is enabled #9225

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ad3f0f
only clear mod component debug logs if logValues is enabled
grahamlangford Oct 3, 2024
824de8e
adds tests
grahamlangford Oct 3, 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
16 changes: 11 additions & 5 deletions src/contentScript/contentScriptPlatform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,17 @@ class ContentScriptPlatform extends PlatformBase {

override get debugger(): PlatformProtocol["debugger"] {
return {
async clear(componentId: UUID): Promise<void> {
await Promise.all([
traces.clear(componentId),
clearModComponentDebugLogs(componentId),
]);
async clear(
componentId: UUID,
{ logValues }: { logValues: boolean },
): Promise<void> {
const clearPromises = [traces.clear(componentId)];

if (logValues) {
clearPromises.push(clearModComponentDebugLogs(componentId));
}

await Promise.all(clearPromises);
},
traces: {
enter: traces.addEntry,
Expand Down
16 changes: 11 additions & 5 deletions src/extensionPages/extensionPagePlatform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,17 @@ class ExtensionPagePlatform extends PlatformBase {
// Support tracing for bricks run in the sidebar and clearing logs in Page Editor/Extension Console. See PanelBody.tsx
override get debugger(): PlatformProtocol["debugger"] {
return {
async clear(componentId: UUID): Promise<void> {
await Promise.all([
traces.clear(componentId),
clearModComponentDebugLogs(componentId),
]);
async clear(
componentId: UUID,
{ logValues }: { logValues: boolean },
): Promise<void> {
const clearPromises = [traces.clear(componentId)];

if (logValues) {
clearPromises.push(clearModComponentDebugLogs(componentId));
}

await Promise.all(clearPromises);
},
traces: {
enter: traces.addEntry,
Expand Down
5 changes: 4 additions & 1 deletion src/platform/platformTypes/debuggerProtocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ export interface DebuggerProtocol {
*
* @param componentId the mod component id
*/
clear: (componentId: UUID) => Promise<void>;
clear: (
componentId: UUID,
{ logValues }: { logValues: boolean },
Copy link
Collaborator

@fungairino fungairino Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could consider renaming this option to something like clearDebugLogs for clarity

Another consideration is whether to expose this in the protocol interface, or just get the logValues value in the underlying implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per our sync discussion, we're going to keep it as logValues for consistency. And the implementation doesn't have access to the source for logValues, so we're leaving that to reducePipeline

) => Promise<void>;

traces: TraceProtocol;
}
16 changes: 11 additions & 5 deletions src/runtime/reducePipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,14 @@ type RunBrickOptions = CommonOptions & {
trace: TraceMetadata;
};

async function getLogValues(
logValues: RunBrickOptions["logValues"] | undefined,
): Promise<boolean> {
const globalLoggingConfig = await getLoggingConfig();

return logValues ?? globalLoggingConfig.logValues ?? false;
}

/**
* Get the lexical environment for running a pipeline. Currently, we're just tracking on the pipeline arg itself.
* https://en.wikipedia.org/wiki/Closure_(computer_programming)
Expand Down Expand Up @@ -456,11 +464,9 @@ async function renderBrickArg(
): Promise<RenderedArgs> {
const { config, type } = resolvedConfig;

const globalLoggingConfig = await getLoggingConfig();

const {
// If logValues not provided explicitly, default to the global setting
logValues = globalLoggingConfig.logValues ?? false,
logValues = await getLogValues(options.logValues),
logger,
explicitArg,
explicitDataFlow,
Expand Down Expand Up @@ -637,7 +643,6 @@ async function applyReduceDefaults({
Partial<ReduceOptions>,
"modComponentRef"
>): Promise<ReduceOptions> {
const globalLoggingConfig = await getLoggingConfig();
const logger = providedLogger ?? new ConsoleLogger();

return {
Expand All @@ -652,7 +657,7 @@ async function applyReduceDefaults({
explicitDataFlow: false,
extendModVariable: false,
// If logValues not provided explicitly, default to the global setting
logValues: logValues ?? globalLoggingConfig.logValues ?? false,
logValues: await getLogValues(logValues),
// For stylistic consistency, default here instead of destructured parameters
branches: [],
// NOTE: do not set runId here. It should be set by the starter brick explicitly, or implicitly generated
Expand Down Expand Up @@ -960,6 +965,7 @@ export async function reduceModComponentPipeline(
// `await` promise to avoid race condition where the calls here delete entries from this call to reducePipeline
await platform.debugger.clear(
partialOptions.modComponentRef.modComponentId,
{ logValues: await getLogValues(partialOptions.logValues) },
);
} catch {
// NOP
Expand Down
Loading