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.

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

feat: cns logger v2 [2/2] #3438

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: cns logger v2 [2/2] #3438

wants to merge 3 commits into from

Conversation

rbtr
Copy link
Contributor

@rbtr rbtr commented Feb 21, 2025

Adds v2 logger package for CNS. [2/2]

The v2 logger is built with zap and offers structured logging, derived contexts, multiple output targets and formats including stdout, files, ETW, and AppInsights.
The logger constructor is opinionated and will initialize targets and formats fully based on what is supported per-platform. The logger is intended to be used directly.

This pt. 2 change adds a migration path to the v2 logger via CNS config. EnableLoggerV2=true replaces the global logger during CNS init with a shimmed v2 logger.

@Copilot Copilot bot review requested due to automatic review settings February 21, 2025 00:12
@rbtr rbtr requested review from a team as code owners February 21, 2025 00:12
@rbtr rbtr requested review from QxBytes and paulyufan2 February 21, 2025 00:12
@rbtr
Copy link
Contributor Author

rbtr commented Feb 21, 2025

Changes in #3433 and #3437 will drop out as those merge

@rbtr rbtr self-assigned this Feb 21, 2025
@rbtr rbtr added the cns Related to CNS. label Feb 21, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

cns/logger/v2/cores/ai.go:51

  • [nitpick] The field keys in the AI core use mixed casing (e.g., 'user_id' vs. 'AppName'). Consider standardizing the naming convention (such as using all lowercase) for consistency.
zap.String("AppName", "name"),

cns/service/main.go:632

  • Passing an empty configuration to loggerv2.New may result in uninitialized logger targets. Consider populating the Config fields explicitly to ensure all intended logging outputs are properly set up.
z, c, err := loggerv2.New(&loggerv2.Config{})
@rbtr rbtr force-pushed the feat/cns-logger-v2 branch 3 times, most recently from 7c254ab to 6745a1b Compare February 27, 2025 21:45
@rbtr rbtr force-pushed the feat/cns-logger-v2 branch from 6745a1b to 0c7f2ee Compare February 27, 2025 21:46
@rbtr rbtr requested a review from Copilot February 28, 2025 03:14

Choose a reason for hiding this comment

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

PR Overview

This PR introduces the v2 logger package for CNS by providing a shim to migrate from the legacy logger and updating configuration and service components to support the new logger.

  • Adds a new v2 logger package built on zap with support for multiple output targets and formats.
  • Provides a migration path via the CNS config with the EnableLoggerV2 flag to swap in the new logger during CNS initialization.
  • Updates various components (service initialization, configuration, legacy logger wrappers) to integrate the v2 logger.

Reviewed Changes

File Description
cns/logger/v2/shim.go Adds shim methods mapping legacy logger calls to zap logger.
cns/service/main.go Migrates logger initialization and updates usage in CNS init.
cns/logger/log.go Deprecates global logger functions in favor of the v2 approach.
cns/logger/v2/logger.go Provides the new zap-based logger implementation.
cns/logger/cnslogger.go Renames legacy logger type and deprecates its usage.
cns/configuration/configuration.go Adds new logger configuration fields for enabling logger v2.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

cns/logger/cnslogger.go:20

  • [nitpick] Consider renaming the 'logger' type to a more descriptive name, such as 'legacyLogger', to clearly convey that this implementation is deprecated and to reduce potential confusion with the global logger.
type logger struct {
@rbtr rbtr requested a review from Copilot February 28, 2025 04:03
@rbtr rbtr enabled auto-merge February 28, 2025 04:06

Choose a reason for hiding this comment

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

PR Overview

This pull request migrates CNS logging to a new v2 logger implementation built with Zap and provides a migration path via the CNS configuration flag EnableLoggerV2. Key changes include:

  • Introduction of the v2 logger package (with shim support) that integrates multiple logging targets.
  • Updates to the main service initialization to use the new logger and pass additional metadata.
  • Adjustments in telemetry and configuration to support the new v2 logging fields and defaults.

Reviewed Changes

File Description
cns/logger/v2/shim.go Adds a shim wrapper to adapt Zap logger to legacy CNS logging API.
cns/logger/v2/fields.go Provides conversion from metadata to zap fields.
cns/service/main.go Updates logger initialization and integration with CNS service.
cns/logger/log.go Deprecates global logger and updates logging API to use v2 logger.
cns/logger/v2/cores/* Implements cores for various outputs (ETW, stdout, file, AI).
cns/logger/v2/logger.go Introduces the new v2 logger constructor based on Zap.
cns/logger/cnslogger.go Renames and migrates the legacy logger to integrate with the v2 shim.
cns/configuration/configuration.go Adds the EnableLoggerV2 flag and logger configuration for v2.
aitelemetry/telemetrywrapper* Updates telemetry file handling by consolidating/removing OS-specific files.

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

cns/logger/v2/cores/file.go:18

  • The 'Fields' field in FileConfig is declared but not applied when building the file core. Consider attaching these fields (e.g. via core.With(cfg.Fields)) to ensure consistency with other cores.
Fields     []zapcore.Field `json:"fields"`

aitelemetry/telemetrywrapper_windows.go:1

  • The OS-specific telemetry wrapper file for Windows has been removed. Ensure that any platform-specific functionality previously provided here is now fully handled by the consolidated telemetry implementation.
package aitelemetry // file removed

aitelemetry/telemetrywrapper_linux.go:1

  • The OS-specific telemetry wrapper file for Linux has been removed. Verify that the consolidated telemetry implementation covers all necessary Linux-specific logic to prevent potential platform issues.
package aitelemetry // file removed
@rbtr
Copy link
Contributor Author

rbtr commented Feb 28, 2025

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cns Related to CNS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant