Skip to content

add logs - #1413

Draft
sahusanket wants to merge 1 commit into
release/6.10from
test_logs_ui_graphql_610
Draft

add logs#1413
sahusanket wants to merge 1 commit into
release/6.10from
test_logs_ui_graphql_610

Conversation

@sahusanket

Copy link
Copy Markdown
Contributor

TITLE

Description

Summary of changes

PR Type

  • Bug Fix
  • Feature
  • Build Fix
  • Testing
  • General Improvement
  • Cherry Pick

Links

Jira: Jira issue #

Test Plan

Screenshots

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces structured logging using log4js across several GraphQL resolvers, batch endpoints, and request helpers, adding a unique queryId to trace incoming requests and backend operations. It also adds a default 5-minute timeout for backend requests. The review feedback highlights a potential performance and log-flooding issue where logging at the info level inside individual pipeline resolvers (nextRuntime, pipelineRuns, and totalRuns) results in O(N) log entries. It is recommended to downgrade these specific resolver-level logs to debug level, as the batch DataLoader endpoints already log the aggregated dispatches.

Comment on lines +37 to +44
log.info(`[Query:${queryId}] Queueing nextRuntime load for pipeline: ${name}`);

const nextRuntimeInfo = await context.loaders.nextRuntime.load({
namespace,
program,
});

log.info(`[Query:${queryId}] Resolved nextRuntime for pipeline: ${name}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since this resolver is executed for each individual pipeline in a list, logging at info level here will generate $O(N)$ log entries (where $N$ is the number of pipelines). This can severely flood the application logs and degrade performance.

Since the DataLoader batch endpoint already logs the batched dispatch (which is much cleaner and grouped), we should change these individual queue/resolve logs to debug level or remove them entirely.

Suggested change
log.info(`[Query:${queryId}] Queueing nextRuntime load for pipeline: ${name}`);
const nextRuntimeInfo = await context.loaders.nextRuntime.load({
namespace,
program,
});
log.info(`[Query:${queryId}] Resolved nextRuntime for pipeline: ${name}`);
log.debug(`[Query:\${queryId}] Queueing nextRuntime load for pipeline: \${name}`);
const nextRuntimeInfo = await context.loaders.nextRuntime.load({
namespace,
program,
});
log.debug(`[Query:\${queryId}] Resolved nextRuntime for pipeline: \${name}`);

Comment on lines +37 to +44
log.info(`[Query:${queryId}] Queueing programRuns load for pipeline: ${name}`);

const runInfo = await context.loaders.programRuns.load({
namespace,
program,
});

log.info(`[Query:${queryId}] Resolved programRuns for pipeline: ${name}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since this resolver is executed for each individual pipeline in a list, logging at info level here will generate $O(N)$ log entries. This can flood the logs and impact performance.

Since the DataLoader batch endpoint already logs the batched dispatch, we should change these individual queue/resolve logs to debug level or remove them entirely.

Suggested change
log.info(`[Query:${queryId}] Queueing programRuns load for pipeline: ${name}`);
const runInfo = await context.loaders.programRuns.load({
namespace,
program,
});
log.info(`[Query:${queryId}] Resolved programRuns for pipeline: ${name}`);
log.debug(`[Query:\${queryId}] Queueing programRuns load for pipeline: \${name}`);
const runInfo = await context.loaders.programRuns.load({
namespace,
program,
});
log.debug(`[Query:\${queryId}] Resolved programRuns for pipeline: \${name}`);

Comment on lines +37 to +44
log.info(`[Query:${queryId}] Queueing totalRuns load for pipeline: ${name}`);

const runInfo = await context.loaders.totalRuns.load({
namespace,
program,
});

log.info(`[Query:${queryId}] Resolved totalRuns for pipeline: ${name}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since this resolver is executed for each individual pipeline in a list, logging at info level here will generate $O(N)$ log entries. This can flood the logs and impact performance.

Since the DataLoader batch endpoint already logs the batched dispatch, we should change these individual queue/resolve logs to debug level or remove them entirely.

Suggested change
log.info(`[Query:${queryId}] Queueing totalRuns load for pipeline: ${name}`);
const runInfo = await context.loaders.totalRuns.load({
namespace,
program,
});
log.info(`[Query:${queryId}] Resolved totalRuns for pipeline: ${name}`);
log.debug(`[Query:\${queryId}] Queueing totalRuns load for pipeline: \${name}`);
const runInfo = await context.loaders.totalRuns.load({
namespace,
program,
});
log.debug(`[Query:\${queryId}] Resolved totalRuns for pipeline: \${name}`);

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.

1 participant