karmada icon indicating copy to clipboard operation
karmada copied to clipboard

refactor: enable JSON logging in authcontroller

Open abhinav-1305 opened this issue 6 months ago • 4 comments

What type of PR is this?

What this PR does / why we need it:

JSON logging support. More details are highlighted in https://github.com/karmada-io/karmada/issues/6230

Which issue(s) this PR fixes:

Part of https://github.com/karmada-io/karmada/issues/6230

Special notes for your reviewer: NA

Does this PR introduce a user-facing change?:

NONE

abhinav-1305 avatar Jun 16 '25 02:06 abhinav-1305

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign xishanyongye-chang for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot avatar Jun 16 '25 02:06 karmada-bot

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 49.05%. Comparing base (cf1a15c) to head (d8a03dc).

Files with missing lines Patch % Lines
...controllers/unifiedauth/unified_auth_controller.go 50.00% 9 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6458   +/-   ##
=======================================
  Coverage   49.04%   49.05%           
=======================================
  Files         687      687           
  Lines       56031    56038    +7     
=======================================
+ Hits        27478    27487    +9     
+ Misses      26772    26770    -2     
  Partials     1781     1781           
Flag Coverage Δ
unittests 49.05% <50.00%> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Jun 16 '25 03:06 codecov-commenter

please review - @jabellard

abhinav-1305 avatar Jun 16 '25 05:06 abhinav-1305

kindly ping @jabellard

RainbowMango avatar Jun 23 '25 01:06 RainbowMango

Hi @RainbowMango - can this be merged in?

abhinav-1305 avatar Jun 23 '25 06:06 abhinav-1305

Well, this PR tries to ingest the information injected by the controller-runtime framework. I like this idea, so that we can have more useful information in log. But this also brings concerns like hard to maintain a consistent logger in the whole code base. So, now let's focus on migrating the existing logs to structural logs, just like what we did at #6435.

More details, you can refer to the discussion at https://github.com/karmada-io/karmada/pull/6435#discussion_r2149147779.

Let me know what you think.

RainbowMango avatar Jun 23 '25 06:06 RainbowMango

Sure, let's start with the log migration like we did in #6435.

abhinav-1305 avatar Jun 23 '25 07:06 abhinav-1305

Sure, let's start with the log migration like we did in #6435.

@abhinav-1305 , please take a look and address when you get a change.

jabellard avatar Jul 01 '25 11:07 jabellard

/assign

jabellard avatar Jul 01 '25 11:07 jabellard

Sure, let's start with the log migration like we did in #6435.

@abhinav-1305 , please take a look and address when you get a change.

Hey @abhinav-1305 , any updates?

jabellard avatar Aug 03 '25 16:08 jabellard

kindly ping @abhinav-1305~

@jabellard is this the last item on the tracking list?

RainbowMango avatar Aug 18 '25 12:08 RainbowMango

kindly ping @abhinav-1305~

@jabellard is this the last item on the tracking list?

Yes. I've allowed ample grace period and am hoping @abhinav-1305 can complete this task, but have not heard back in over a month.

Will allow some more time by waiting a few days. If I don't hear back by then, I have someone who's interested in contributing and will pass the ticket along so we can wrap up the feature on time.

jabellard avatar Aug 18 '25 12:08 jabellard

Sounds good to me. I hope this will be included in the release v1.15.0, which will be cut on August 31st, by the way.

RainbowMango avatar Aug 18 '25 12:08 RainbowMango

Will allow some more time by waiting a few days. If I don't hear back by then, I have someone who's interested in contributing and will pass the ticket along so we can wrap up the feature on time.

@jabellard it's time to go with plan B :)

RainbowMango avatar Aug 27 '25 01:08 RainbowMango

Will allow some more time by waiting a few days. If I don't hear back by then, I have someone who's interested in contributing and will pass the ticket along so we can wrap up the feature on time.

@jabellard it's time to go with plan B :)

Yup. Will push PR in a few to close the party!

jabellard avatar Aug 27 '25 01:08 jabellard

/close in favor of #6700

RainbowMango avatar Aug 27 '25 01:08 RainbowMango

@RainbowMango: Closed this PR.

In response to this:

/close in favor of #6700

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

karmada-bot avatar Aug 27 '25 01:08 karmada-bot