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

chore: upgrade fx event logger #4036

Merged
merged 7 commits into from
May 10, 2023

Conversation

g1eny0ung
Copy link
Member

@g1eny0ung g1eny0ung commented May 7, 2023

What's changed and how it works?

After Fx v1.14.0, there is a new method (fx.WithLogger) to add a custom logger to log Fx's own operations (see https://uber-go.github.io/fx/get-started/logger.html) and the old method is deprecated. We used to use a simple Printf wrapper, now I create a logr adapter to seamlessly integrate our zapr logger.

Related changes

  • This change also requires further updates to the website (e.g. docs)
  • This change also requires further updates to the UI interface
  • Need to cheery-pick to release branches
    • release-2.5
    • release-2.4

Checklist

CHANGELOG

  • I have updated the CHANGELOG.md
  • I have labeled this PR with "no-need-update-changelog"

Tests

  • Unit test
  • E2E test
  • No code
  • Manual test (add steps below)

Side effects

  • Breaking backward compatibility

DCO

If you find the DCO check fails, please run commands like below (Depends on the actual situations. For example, if the failed commit isn't the most recent) to fix it:

git commit --amend --signoff
git push --force

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung g1eny0ung added the type/enhancement New feature or request label May 7, 2023
@g1eny0ung g1eny0ung requested a review from STRRL May 7, 2023 04:06
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@codecov
Copy link

codecov bot commented May 7, 2023

Codecov Report

Merging #4036 (45814a7) into master (e2f2292) will increase coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 45814a7 differs from pull request most recent head 4bb7856. Consider uploading reports for the commit 4bb7856 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4036      +/-   ##
==========================================
+ Coverage   38.61%   38.69%   +0.08%     
==========================================
  Files         167      167              
  Lines       13734    13734              
==========================================
+ Hits         5303     5314      +11     
+ Misses       7996     7989       -7     
+ Partials      435      431       -4     

see 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29335e8...4bb7856. Read the comment docs.

@STRRL
Copy link
Member

STRRL commented May 7, 2023

basically LGTM, but I am not sure about creating a new module as a new standalone GitHub Repository (I mean https://github.com/chaos-mesh/fx-logr) with pseudo-version in go.mod

What about

  • tag on the chaos-mesh/fx-logr and mark it as a released version, never mind 0.0.1 or 0.1.0 as a pre-rereleased version.
  • Or, NOT using a standalone GitHub Repo

What do you think about it? @g1eny0ung

@g1eny0ung
Copy link
Member Author

tag on the chaos-mesh/fx-logr and mark it as a released version, never mind 0.0.1 or 0.1.0 as a pre-rereleased version.

@STRRL Makes sense. Yes, I forgot to tag a release version to it. I will tag it now.

As for why I created a standalone repo, it's because I think it's a generic adapter, not related to Chaos Mesh.

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung g1eny0ung requested a review from cwen0 May 8, 2023 05:38
@STRRL
Copy link
Member

STRRL commented May 9, 2023

/lgtm

@chaotic-prow
Copy link

chaotic-prow bot commented May 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: STRRL

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

The pull request process is described 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

@g1eny0ung
Copy link
Member Author

/retest

@g1eny0ung g1eny0ung merged commit 7c75a0e into chaos-mesh:master May 10, 2023
@g1eny0ung g1eny0ung deleted the chore/upgrade-fx-event-logger branch May 10, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants