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

ETW logging implementation in CNI #2668

Merged
merged 21 commits into from
Apr 18, 2024
Merged

ETW logging implementation in CNI #2668

merged 21 commits into from
Apr 18, 2024

Conversation

sivakami-projects
Copy link
Contributor

@sivakami-projects sivakami-projects commented Apr 2, 2024 •
edited
Loading

Feat: Features 🌈 Added support for logging to ETW using zap logger.

Reason for Change:

  • Implemented logging to ETW using the zap logger to provide a unified logging experience in Azure Container Networking.
  • This feature introduces a zap writer syncer capable of writing to ETW, facilitating more efficient debugging and monitoring.

Requirements:

Notes:

  • The implementation introduces a zap core that directs logging output to ETW.
  • ETW logs are written for all windows machines.
  • This configuration allows Azure Container Networking to log simultaneously to ETW and the file system, ensuring that logs are accessible through ETW while still being persisted to files.
  • This addition is designed to be transparent to existing logging mechanisms and does not interfere with the current logging functionality.

@sivakami-projects sivakami-projects requested review from a team as code owners April 2, 2024 16:31
@sivakami-projects sivakami-projects changed the title [DRAFT] ETW logging implementation in CNI [DRAFT] [WIP] ETW logging implementation in CNI Apr 2, 2024
@sivakami-projects sivakami-projects marked this pull request as draft April 2, 2024 16:33
@sivakami-projects sivakami-projects self-assigned this Apr 2, 2024
@sivakami-projects sivakami-projects added cni Related to CNI. logging labels Apr 2, 2024
timraymond
timraymond reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just had a few thoughts.

cni/log/logger.go Outdated Show resolved Hide resolved
cni/log/logger.go Outdated Show resolved Hide resolved
zapetw/write_syncer.go Outdated Show resolved Hide resolved
zapetw/write_syncer.go Outdated Show resolved Hide resolved
@sivakami-projects sivakami-projects requested review from paulyufan2 and removed request for tamilmani1989 April 2, 2024 19:36
paulyufan2
paulyufan2 reviewed Apr 3, 2024
View reviewed changes
zapetw/write_syncer_windows.go Outdated Show resolved Hide resolved
zapetw/write_syncer_windows.go Outdated Show resolved Hide resolved
zapetw/core_windows.go Outdated Show resolved Hide resolved
zapetw/core_windows.go Outdated Show resolved Hide resolved
@paulyufan2 paulyufan2 requested a review from tamilmani1989 April 3, 2024 19:40
@sivakami-projects sivakami-projects force-pushed the CNI_ETW_Sivakami_Logger_Branch branch from 5fe906d to a408dc5 Compare April 3, 2024 20:33
rbtr
rbtr reviewed Apr 3, 2024
View reviewed changes
Copy link
Contributor

@rbtr rbtr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

some initial comments

zapetw/unsupported_linux.go Outdated Show resolved Hide resolved
zapetw/write_syncer_windows.go Outdated Show resolved Hide resolved
zapetw/core_windows.go Outdated Show resolved Hide resolved
zapetw/core_windows.go Show resolved Hide resolved
zapetw/core_windows.go Outdated Show resolved Hide resolved
zapetw/core_windows.go Show resolved Hide resolved
zapetw/write_syncer_windows.go Outdated Show resolved Hide resolved
zapetw/write_syncer_windows.go Outdated Show resolved Hide resolved
@sivakami-projects sivakami-projects marked this pull request as ready for review April 8, 2024 16:59
@sivakami-projects sivakami-projects changed the title [DRAFT] [WIP] ETW logging implementation in CNI ETW logging implementation in CNI Apr 8, 2024
@paulyufan2
Copy link
Contributor

lgtm, please get approval from Evan and TM.

paulyufan2
paulyufan2 previously approved these changes Apr 8, 2024
View reviewed changes
@paulyufan2
Copy link
Contributor

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

rbtr
rbtr requested changes Apr 10, 2024
View reviewed changes
cni/log/logger_linux.go Outdated Show resolved Hide resolved
cni/log/logger_windows.go Outdated Show resolved Hide resolved
cni/log/logger.go Outdated Show resolved Hide resolved
rbtr
rbtr previously approved these changes Apr 11, 2024
View reviewed changes
Copy link
Contributor

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

couple nits, mostly LGTM

cni/log/logger.go Outdated Show resolved Hide resolved
cni/log/logger.go Outdated Show resolved Hide resolved
sivakami.sde@gmail.com added 16 commits April 16, 2024 09:21
@sivakami-projects
1. Implemented platform-specific ETW logging enhancements.
8c11672 
2. Refactor ETW initialization into dedicated method and zapetw package.
@sivakami-projects
Changed InitETWLogger method signature for Linux.
99aad08
@sivakami-projects
Wrapped error messages at each level of the call hierarchy.
69d932b
@sivakami-projects
Removed punctuation marks from error messages.
6aa0a9a
@sivakami-projects
Wrapped error messages with errors.wrap method.
4b1e089
@sivakami-projects
Added comments for clarity.
8a02e93
@sivakami-projects
implemented zap.core for ETW.
4bbfe82
@sivakami-projects
Fixed lint issues.
ee1ba36
@sivakami-projects
Catch errors from etw.writeEvent method.
7b1076c
@sivakami-projects
Renamed provider.
2cc51b2
@sivakami-projects
Abstracted etw core creation in logger_windows.
b178751 
Removed unsupported error from logger_linux to keep the behaviour uniform.
@sivakami-projects
renamed unused parameter.
2b19c04
@sivakami-projects
Renamed variable to lower camel case as it is private. Removed additi…
82b587a 
…onal local reference.
@sivakami-projects
fixed variable name.
612a914
@sivakami-projects
Added comment.
2e191b8
@sivakami-projects
Renamed ETW provider, removed application names from the provider name.
f75e2a2
@sivakami-projects sivakami-projects force-pushed the CNI_ETW_Sivakami_Logger_Branch branch from 7f99728 to f75e2a2 Compare April 16, 2024 16:21
@sivakami-projects
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sivakami-projects sivakami-projects added this pull request to the merge queue Apr 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2024
@sivakami-projects sivakami-projects added this pull request to the merge queue Apr 17, 2024
@sivakami-projects sivakami-projects removed this pull request from the merge queue due to a manual request Apr 17, 2024
@sivakami-projects sivakami-projects added this pull request to the merge queue Apr 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 18, 2024
@sivakami-projects sivakami-projects added this pull request to the merge queue Apr 18, 2024
Merged via the queue into Azure:master with commit 1eed84e Apr 18, 2024
12 checks passed
@sivakami-projects sivakami-projects deleted the CNI_ETW_Sivakami_Logger_Branch branch April 18, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulyufan2 paulyufan2 paulyufan2 approved these changes

@rbtr rbtr rbtr approved these changes

@tamilmani1989 tamilmani1989 tamilmani1989 approved these changes

@timraymond timraymond Awaiting requested review from timraymond

Assignees

@sivakami-projects sivakami-projects

Labels
cni Related to CNI. logging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sivakami-projects @paulyufan2 @tamilmani1989 @timraymond @rbtr