Introduce internal Console API to encapsulate console related calls to manage them from a single point

[serkan-ozal]

Which problem is this PR solving?

This PR neither adds new functionality or fixes a bug, but refactors the code base to use console methods over internal Console API instead of calling console methods directly.

This can be helpful for us in the future (open-telemetry/opentelemetry-js-contrib#1560) once we add patching console methods to automatically exporting them as logs. The point is that, we may not want to capture our own console exports for traces, metrics and logs to the console and export them as log again. So, when we patch the console methods, we can configure Console API (over Console.configure()) with the original unpatched console methods, so they will not be intercepted.

Short description of the changes

All the direct console calls are refactored to be done over internal Console API. So, when needed, console log behaviors can be managed from this single point.

Type of change

Neither fix, nor new feature, but refactor for further new features.

How Has This Been Tested?

Existing tests of the following files test the changes in this PR:

• ConsoleTraceExporter
• ConsoleMetricExporter
• ConsoleLogRecordExporter

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[pichlermarc]

Hmm, I think I don't fully understand. If a user explicitly configures the console exporter (or the console diag logger), it stands to reason that the user would also expect their output to also show up in the exported logs, no?

Or is this about resolving a potential footgun that we provide to users with the SimpleLogRecordProcesor ending up in an infinite loop when having diag logging/console exporter exporters enabled?

[pichlermarc]

this is a breaking change that we cannot do in a minor version.

[serkan-ozal]

Any idea to not to make this breaking change or if these changes make sense, do we need to wait for v2?

[serkan-ozal]

@pichlermarc Maybe this should not be in the API?

[serkan-ozal]

Does this make sense?

• Move this internal Console API to @opentelemetry/core
• Keep diag/consoleLogger as is (will still access console directly), because @opentelemetry/api can't have a dependency to @opentelemetry/core so Console API is not visible to the @opentelemetry/api

[serkan-ozal]

@pichlermarc Moved to @opentelemetry/core, so no breaking change over @opentelemetry/api.

[serkan-ozal]

Hi @pichlermarc,

Actually, this introduced API is intended to be internal, not for the users. We will not expect users to log over Console API, but we will use it internally to log anything to console.

The motivation is that (in the future) when/if we patch the console log methods (log, trace, debug, info, warn, error, dir) to capture console logs and export them as OTEL log records automatically, we will not want to capture console logs printed by console exporters of trace, metrics and logs.

So the solution over this API might be that

• Get references of the original console log methods (log, trace, debug, info, warn, error, dir)
• Patch/wrap console log methods to export console log messages/args to log exporter as OTEL log records
• Configure internal Console API to use the original console log methods we got in the first step above. So, since console exporters of trace, metrics and logs will log over internal Console API, those console logs will not be captured our patched console logs, because they will be forwarded to original console log methods.

For example:

const origConsoleLogFunction = console.log;
const origConsoleErrorFunction = console.error;
const origConsoleWarnFunction = console.warn;
const origConsoleInfoFunction = console.info;
const origConsoleDebugFunction = console.debug;
const origConsoleTraceFunction = console.trace;
const origConsoleDirFunction = console.dir;

// Patch/wrap console log methods

Console.configure({
  log: origConsoleLogFunction,
  error: origConsoleErrorFunction,
  warn: origConsoleWarnFunction,
  info: origConsoleInfoFunction,
  debug: origConsoleDebugFunction,
  trace: origConsoleTraceFunction,
  dir: origConsoleDirFunction,
}, console);

Hope that this explains the motivation in more clear way. So does these changes make sense?

[serkan-ozal]

Hi @pichlermarc, any update on this?

[dyladan]

use console methods over internal Console API instead of calling console methods directly.

I think there's a typo in here but i can't figure out what you were trying to say

---

@pichlermarc I think this is trying to prevent a loop if we capture logs from console and also export to console. Right now we are calling console.dir directly in the console exporters. This PR calls methods on a class which wraps console allowing us to suppress capturing of them during export.

[dyladan]

I think the same thing can be achieved by setting a context property when logging that suppresses capture. We do the same thing to suppress tracing our http/grpc exporters.

[dyladan]

Not clear what this is for

[serkan-ozal]

it is the configured object to be used while calling console function with the apply

[dyladan]

We try to avoid any when possible. Also not clear what obj is for

[serkan-ozal]

it is the object to be used while calling console function with apply above

[serkan-ozal]

@dyladan

I think this is trying to prevent a loop if we capture logs from console and also export to console. Right now we are calling console.dir directly in the console exporters. This PR calls methods on a class which wraps console allowing us to suppress capturing of them during export.

Yes, preventing the loop is the original motivation of these changes.

[serkan-ozal]

I think the same thing can be achieved by setting a context property when logging that suppresses capture. We do the same thing to suppress tracing our http/grpc exporters.

@dyladan actually, I also thought the same thing, but I think, even to set context property to suppress just before internal logging, we will still need to have some sort of internal API or module to do that and check whether it is set to decide suppress or not in the patched console methods. So, instead of doing this, always using this internal console API for internal logs made more sense to me.

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.