Skip to content

[TT-7099] [Customer FR] Send info and debug messages to stdout not stderr #1024

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

Open
shults wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[TT-7099] [Customer FR] Send info and debug messages to stdout not stderr #1024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions serializer/serializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,56 @@

import (
"github.com/TykTechnologies/tyk-pump/analytics"
logger "github.com/TykTechnologies/tyk-pump/logger"
"github.com/TykTechnologies/tyk-pump/logger"
"github.com/sirupsen/logrus"
)

var log = logger.GetLogger()

type AnalyticsSerializer interface {
Encode(record *analytics.AnalyticsRecord) ([]byte, error)
Decode(analyticsData interface{}, record *analytics.AnalyticsRecord) error
GetSuffix() string
}

const MSGP_SERIALIZER = "msgpack"
const PROTOBUF_SERIALIZER = "protobuf"
const (
MSGP_SERIALIZER = "msgpack"
PROTOBUF_SERIALIZER = "protobuf"
)

type analyticsSerializerOptions struct {
logger *logrus.Logger
}

type NewAnalyticsSerializerOpt func(*analyticsSerializerOptions)

func NewAnalyticsSerializer(
serializerType string,
options ...NewAnalyticsSerializerOpt,
) AnalyticsSerializer {
opt := analyticsSerializerOptions{
logger: logger.GetLogger(),
}

for _, apply := range options {
apply(&opt)
}

func NewAnalyticsSerializer(serializerType string) AnalyticsSerializer {
switch serializerType {
case PROTOBUF_SERIALIZER:
serializer := &ProtobufSerializer{}
log.Debugf("Using serializer %v for analytics \n", PROTOBUF_SERIALIZER)
opt.logger.Debugf("Using serializer %v for analytics \n", PROTOBUF_SERIALIZER)
return serializer
case MSGP_SERIALIZER:
fallthrough

Check warning on line 44 in serializer/serializer.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The `case MSGP_SERIALIZER` with `fallthrough` is redundant. The `default` case already provides the desired behavior for `MSGP_SERIALIZER` and any other unspecified serializer type. This can be simplified by removing the explicit case for `MSGP_SERIALIZER`.
Raw output 
Remove the `case MSGP_SERIALIZER:` and `fallthrough` to simplify the switch statement, as the `default` case handles this logic.
default:
log.Debugf("Using serializer %v for analytics \n", MSGP_SERIALIZER)
opt.logger.Debugf("Using serializer %v for analytics \n", MSGP_SERIALIZER)
return &MsgpSerializer{}
}
}

// WithLogger

Check warning on line 51 in serializer/serializer.go

View check run for this annotation

probelabs / Visor: quality

logic Issue 

The log message in the default case is misleading. When an unknown `serializerType` is provided, the code falls through to the `default` block, which logs that the `MSGP_SERIALIZER` is being used without indicating that a fallback occurred. This can be confusing for operators diagnosing configuration issues.
Raw output 
Refactor the switch statement to handle the default case explicitly with a more informative log message that states which serializer was requested and which one is being used as a default. This avoids the `fallthrough` and makes the logging clearer.
// Overrides default logger.
func WithLogger(logger *logrus.Logger) NewAnalyticsSerializerOpt {
return func(o *analyticsSerializerOptions) {
o.logger = logger
}
return &MsgpSerializer{}
}
25 changes: 25 additions & 0 deletions serializer/serializer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
"github.com/TykTechnologies/tyk-pump/analytics/demo"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/sirupsen/logrus"
logrustest "github.com/sirupsen/logrus/hooks/test"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -207,6 +209,29 @@
}
}

func Test_WithLogger(t *testing.T) {
logger, hook := logrustest.NewNullLogger()
logger.SetLevel(logrus.DebugLevel)

t.Run("writes logs to provided logger in case MSGP_SERIALIZER", func(t *testing.T) {
hook.Reset()
_ = NewAnalyticsSerializer(MSGP_SERIALIZER, WithLogger(logger))
assert.True(t, len(hook.AllEntries()) > 0)
})

t.Run("writes logs to provided logger in case PROTOBUF_SERIALIZER", func(t *testing.T) {
hook.Reset()
_ = NewAnalyticsSerializer(PROTOBUF_SERIALIZER, WithLogger(logger))
assert.True(t, len(hook.AllEntries()) > 0)
})

t.Run("writes logs to provided logger in case wrong string", func(t *testing.T) {

Check warning on line 228 in serializer/serializer_test.go

View check run for this annotation

probelabs / Visor: quality

style Issue 

The tests for the injected logger only assert that at least one log entry was created (`len(hook.AllEntries()) > 0`). This is a weak assertion as it doesn't validate the *content* of the log message. A more robust test would verify that the correct message is logged for each serializer type, which would have caught the misleading log message issue in the implementation.
Raw output 
Strengthen the test assertions to check the content of the log messages. Use `require.Len` to ensure exactly one message is logged and `assert.Contains` to verify its content. This will make the tests more effective at catching regressions.
hook.Reset()
_ = NewAnalyticsSerializer("dummy", WithLogger(logger))
assert.True(t, len(hook.AllEntries()) > 0)
})
}

func BenchmarkProtobufEncoding(b *testing.B) {
serializer := NewAnalyticsSerializer(PROTOBUF_SERIALIZER)
records := []analytics.AnalyticsRecord{
Expand Down
Loading