Repository: go-kratos/kratos
Pull request #1901 (2022-03-29 09:17:11)

fix(log): FilterFunc keyvals lost logger prefix
<!--
🎉 Thanks for sending a pull request to Kratos! Here are some tips for you:

1. If this is your first time contributing to Kratos, please read our contribution guide: https://go-kratos.dev/en/docs/community/contribution/
2. Ensure you have added or ran the appropriate tests and lint for your PR, please use `make lint` and `make test` before filing your PR, use `make clean` to tidy your go mod.
3. If the PR is unfinished, you may need mark it as a WIP(Work In Progress) PR or draft PR
4. Please use a conventional commits format title: `<type>[optional scope]: <description>`
    
    Some suggestion on <type>:

    fix: A bug fix
    feat: A new feature
    test: Adding missing tests or correcting existing tests
    refactor: A code change that neither fixes a bug nor adds a feature
    break: Changes has break change

    docs: Documentation only changes
    deps: Changes external dependencies
    style: Changes that do not affect the meaning of the code (white-space, formatting, etc)
    chore Daily work, examples, etc.
    ci: Changes to our CI configuration files and scripts
-->

#### Description (what this PR does / why we need it):
<!--
* The description should include the motivation for this PR or contrast this with previous behavior
-->


#### Which issue(s) this PR fixes (resolves / be part of):
<!--
* Automatically closes linked issue when PR is merged.
* If you PR is not fully resolved issue, please use `part of #<issue number>` instead.

Usage: `fixes/resolves #<issue number>`, or `fixes/resolves (paste link of issue)`.
-->
fixes # https://github.com/go-kratos/kratos/issues/1897

#### Other special notes for reviewer:
<!--
* Somethings that need extra attention for reviewer
* Some additional notes, TODO list, etc.
-->


codecov-commenter: # [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1901?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) Report
> Merging [#1901](https://codecov.io/gh/go-kratos/kratos/pull/1901?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (240b558) into [main](https://codecov.io/gh/go-kratos/kratos/commit/d4c0c576819e29f5103a8b1e3c44d88dcecc7e86?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (d4c0c57) will **decrease** coverage by `0.11%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main    #1901      +/-   ##
==========================================
- Coverage   80.81%   80.70%   -0.12%     
==========================================
  Files          80       80              
  Lines        3565     3570       +5     
==========================================
  Hits         2881     2881              
- Misses        477      481       +4     
- Partials      207      208       +1     
```


| [Impacted Files](https://codecov.io/gh/go-kratos/kratos/pull/1901?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) | Coverage Δ | |
|---|---|---|
| [log/filter.go](https://codecov.io/gh/go-kratos/kratos/pull/1901/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-bG9nL2ZpbHRlci5nbw==) | `95.23% <100.00%> (+0.64%)` | :arrow_up: |
| [selector/node/ewma/node.go](https://codecov.io/gh/go-kratos/kratos/pull/1901/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-c2VsZWN0b3Ivbm9kZS9ld21hL25vZGUuZ28=) | `83.87% <0.00%> (-3.23%)` | :arrow_down: |
| [internal/context/context.go](https://codecov.io/gh/go-kratos/kratos/pull/1901/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-aW50ZXJuYWwvY29udGV4dC9jb250ZXh0Lmdv) | `97.01% <0.00%> (-2.99%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1901?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1901?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Last update [d4c0c57...240b558](https://codecov.io/gh/go-kratos/kratos/pull/1901?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).

kagaya85: we may need to add some unit tests?
Cluas: > we may need to add some unit tests?

This test is quite difficult to write, 😂, anyway, I added some tests. @kagaya85 

Patch:
diff --git a/log/filter.go b/log/filter.go
index 9258f3f3..9ef87ddf 100644
--- a/log/filter.go
+++ b/log/filter.go
@@ -64,7 +64,18 @@ func (f *Filter) Log(level Level, keyvals ...interface{}) error {
 	if level < f.level {
 		return nil
 	}
-	if f.filter != nil && f.filter(level, keyvals...) {
+	// fkv is used to provide a slice to contains both logger.prefix and keyvals for filter
+	var fkv []interface{}
+	if l, ok := f.logger.(*logger); ok {
+		if len(l.prefix) > 0 {
+			fkv = make([]interface{}, 0, len(l.prefix)+len(keyvals))
+			fkv = append(fkv, l.prefix...)
+			fkv = append(fkv, keyvals...)
+		}
+	} else {
+		fkv = keyvals
+	}
+	if f.filter != nil && f.filter(level, fkv...) {
 		return nil
 	}
 	if len(f.key) > 0 || len(f.value) > 0 {


Test patch:
diff --git a/log/filter_test.go b/log/filter_test.go
index 4a8b249f..d3c8d548 100644
--- a/log/filter_test.go
+++ b/log/filter_test.go
@@ -1,6 +1,7 @@
 package log
 
 import (
+	"bytes"
 	"io"
 	"testing"
 )
@@ -89,3 +90,44 @@ func testFilterFunc(level Level, keyvals ...interface{}) bool {
 	}
 	return false
 }
+
+func TestFilterFuncWitchLoggerPrefix(t *testing.T) {
+	buf := new(bytes.Buffer)
+	tests := []struct {
+		logger Logger
+		want   string
+	}{
+		{
+			logger: NewFilter(With(NewStdLogger(buf), "caller", "caller", "prefix", "whaterver"), FilterFunc(testFilterFuncWithLoggerPrefix)),
+			want:   "",
+		},
+		{
+			logger: NewFilter(With(NewStdLogger(buf), "caller", "caller"), FilterFunc(testFilterFuncWithLoggerPrefix)),
+			want:   "INFO caller=caller msg=msg\n",
+		},
+	}
+
+	for _, tt := range tests {
+		err := tt.logger.Log(LevelInfo, "msg", "msg")
+		if err != nil {
+			t.Fatal("err should be nil")
+		}
+		got := buf.String()
+		if got != tt.want {
+			t.Fatalf("filter should catch prefix, want %s, got %s.", tt.want, got)
+		}
+		buf.Reset()
+	}
+}
+
+func testFilterFuncWithLoggerPrefix(level Level, keyvals ...interface{}) bool {
+	if level == LevelWarn {
+		return true
+	}
+	for i := 0; i < len(keyvals); i += 2 {
+		if keyvals[i] == "prefix" {
+			return true
+		}
+	}
+	return false
+}