Skip to content

Conversation

@feelform
Copy link
Collaborator

@feelform feelform commented Oct 22, 2025

[#358] remove unused PinpointClient
[#380] Remove deprecated logLevel configuration usage
[#381] make logger state immutable and inject default logger
[#376] Support ESM

@feelform feelform linked an issue Oct 22, 2025 that may be closed by this pull request
@feelform feelform force-pushed the prepare-v1.3.0 branch 2 times, most recently from 95ba30b to 9aa2119 Compare October 24, 2025 09:29
@feelform feelform self-assigned this Oct 30, 2025
@feelform feelform requested a review from Copilot November 3, 2025 07:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the logging system from a single global logger configuration (PINPOINT_LOG_LEVEL) to a per-logger level configuration system (PINPOINT_LOGGER_LEVELS). The changes enable independent log level management for different loggers (e.g., default logger and gRPC logger).

Key changes:

  • Refactored the Logger class architecture to support multiple named logger instances with a central registry
  • Removed the deprecated PINPOINT_LOG_LEVEL environment variable in favor of PINPOINT_LOGGER_LEVELS
  • Added support for configuring individual logger levels via the new logger-levels configuration property

Reviewed Changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
lib/utils/log/logger.js Restructured Logger and ChildLogger classes; Logger now manages multiple ChildLogger instances in a Map registry
lib/utils/log/log-builder.js Added NoConfigLog class to track config usage; added getName() method and createGrpcLogBuilder() static method
lib/utils/log/types.d.ts Removed setRootLogger method from Logger interface
lib/config.js Removed logLevel property from ENV_MAP and CONFIG_FILE_MAP
lib/config.d.ts Changed loggerLevels from optional to required, removed logLevel property
lib/pinpoint-config-default.json Added grpcLogger to logger-levels configuration with SILENT level
lib/agent-builder.js Added setLogger method to support injecting custom logger instances
lib/client/pinpoint-client.js Deleted file (removed entire PinpointClient class)
index.js Removed setRootLogger call; now creates and injects default logger via AgentBuilder
README.md Removed PINPOINT_LOG_LEVEL documentation
test/utils/log/logger.test.js Updated all test references from log to logger variable; renamed variable to avoid shadowing
test/config.test.js Updated test assertion to expect both default-logger and grpcLogger in logger levels
test/agent/env-config.test.js Updated tests to use PINPOINT_LOGGER_LEVELS instead of PINPOINT_LOG_LEVEL
test/pinpoint-config-test.json Replaced log-level with logger-levels structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

log.appenders = this.appenders
return log
const noConfigLog = new NoConfigLog(this.name, this.level)
noConfigLog.appenders = this.appenders
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The appenders array is assigned directly without cloning, which means all NoConfigLog instances will share the same appenders array reference. This can cause unintended side effects if the array is mutated later. The assignment should clone the array: noConfigLog.appenders = [...this.appenders] or assign appenders in the constructor.

Suggested change
noConfigLog.appenders = this.appenders
noConfigLog.appenders = [...this.appenders]

Copilot uses AI. Check for mistakes.
t.equal(given.applicationName, "appication name", "No set PINPOINT_APPLICATION_NAME env, should equal default config")
t.equal(given.collectorIp, "localhost", "No set PINPOINT_COLLECTOR_IP env, should equal default config")
t.equal(given.logLevel, "WARN", "No set PINPOINT_LOG_LEVEL env, should equal default config")
t.deepEqual(given.loggerLevels, { 'default-logger': 'WARN', 'grpcLogger': 'SILENT' }, "No set PINPOINT_ LOGGER_LEVELS env, should equal default config")
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

There is an extra space in the assertion message. 'PINPOINT_ LOGGER_LEVELS' should be 'PINPOINT_LOGGER_LEVELS'.

Suggested change
t.deepEqual(given.loggerLevels, { 'default-logger': 'WARN', 'grpcLogger': 'SILENT' }, "No set PINPOINT_ LOGGER_LEVELS env, should equal default config")
t.deepEqual(given.loggerLevels, { 'default-logger': 'WARN', 'grpcLogger': 'SILENT' }, "No set PINPOINT_LOGGER_LEVELS env, should equal default config")

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment