TykTechnologies / tyk

Tyk Open Source API Gateway written in Go, supporting REST, GraphQL, TCP and gRPC protocols
Other
9.76k stars 1.09k forks source link

Merging to release-5.3: [TT-12355] added log format config, func and test cases (#6344) #6484

Closed buger closed 2 months ago

buger commented 2 months ago

User description

TT-12355 added log format config, func and test cases (#6344)

FR Jira Ticket

https://tyktech.atlassian.net/browse/TT-12355

Description

Additionally, TYK_LOGFORMAT is supported, as is TYK_LOGLEVEL (and TYK_GW_LOGLEVEL).

Note that there is some global use from the log package, I don't think we can set json output from json config, as it would be picked up too late. The one case to cover is to make sure this passes with only config enabled:

# TYK_GW_LOGFORMAT=json ./tyk 2>&1 | head -n 1
{"level":"info","msg":"Tyk API Gateway v5.3.0-dev","prefix":"main","time":"2024-07-10T15:16:11+02:00"}
# TYK_LOGFORMAT=json ./tyk 2>&1 | head -n 1
{"level":"info","msg":"Tyk API Gateway v5.3.0-dev","prefix":"main","time":"2024-07-10T15:16:14+02:00"}
# ./tyk 2>&1 | head -n 1
{"level":"info","msg":"Tyk API Gateway v5.3.0-dev","prefix":"main","time":"2024-07-10T15:45:02+02:00"}

The above confirms reading from env and from config.

Related Issue

Motivation and Context

The option to determine which STDOUT log format will open the opportunity for customers to easily parse the STDOUT logs and open up opportunity for our team to easily write tests and parse as needed.

How This Has Been Tested

PR Type

Enhancement, Tests


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
log.go
Add log format configuration and initialization                   

log/log.go
  • Added NewFormatter function to handle log format based on
    TYK_GW_LOGFORMAT environment variable.
  • Modified the default log formatter initialization to use NewFormatter.

  • +17/-6   
    Tests
    log_test.go
    Add unit tests for log formatter configuration                     

    log/log_test.go
  • Added unit tests for NewFormatter function.
  • Tested JSON and Text formatters based on input format.
  • +61/-0   

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions


    Co-authored-by: Long Le tyk-longle@Tyk-longle-MacBook-Pro.local Co-authored-by: Tit Petric tit@tyk.io


    PR Type

    enhancement, tests


    Description


    Changes walkthrough ๐Ÿ“

    Relevant files
    Enhancement
    5 files
    config.go
    Add log format configuration option                                           

    config/config.go
  • Added a new LogFormat configuration option.
  • Default log format set to standard.
  • +4/-0     
    server.go
    Refactor system initialization and add log format setup   

    gateway/server.go
  • Refactored initialiseSystem to initSystem.
  • Added log format initialization based on environment variables.
  • Adjusted log level setup to include new log format logic.
  • +31/-21 
    json_formatter.go
    Implement JSON log formatter                                                         

    log/json_formatter.go
  • Introduced a new JSON log formatter.
  • Implemented log entry formatting into JSON.
  • +60/-0   
    log.go
    Enhance logging with format and level configuration           

    log/log.go
  • Added functionality to select log format and level from environment
    variables.
  • Refactored logging setup to support multiple formats.
  • +59/-43 
    translation_formatter.go
    Separate translation formatter logic                                         

    log/translation_formatter.go
  • Moved translation formatter logic to a separate file.
  • Implemented translation-based log message formatting.
  • +36/-0   
    Tests
    2 files
    testutil.go
    Update test setup with new system initialization                 

    gateway/testutil.go - Updated call to `initSystem` in test setup.
    +1/-1     
    log_test.go
    Add tests and benchmarks for log formatters                           

    log/log_test.go
  • Added tests for new log formatter functionality.
  • Benchmarked different log formatters.
  • +70/-0   
    Configuration changes
    2 files
    schema.json
    Update schema with log format option                                         

    cli/linter/schema.json - Added `log_format` configuration option to schema.
    +4/-0     
    Taskfile.yml
    Add Taskfile for build automation                                               

    log/Taskfile.yml
  • Added task definitions for testing, benchmarking, and formatting.
  • Included coverage and linting tasks.
  • +50/-0   
    Dependencies
    2 files
    go.mod
    Add go-json dependency                                                                     

    go.mod - Added `github.com/goccy/go-json` dependency.
    +1/-0     
    go.sum
    Update go.sum with new dependency checksums                           

    go.sum - Updated with checksums for `go-json` dependency.
    +2/-0     

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    github-actions[bot] commented 2 months ago

    API Changes

    --- prev.txt    2024-09-05 11:17:07.003883236 +0000
    +++ current.txt 2024-09-05 11:17:03.911857676 +0000
    @@ -5247,6 +5247,10 @@
        // If not set or left empty, it will default to `info`.
        LogLevel string `json:"log_level"`
    
    +   // You can now configure the log format to be either the standard or json format
    +   // If not set or left empty, it will default to `standard`.
    +   LogFormat string `json:"log_format"`
    +
        // Section for configuring OpenTracing support
        // Deprecated: use OpenTelemetry instead.
        Tracer Tracer `json:"tracing"`
    @@ -8121,8 +8125,6 @@
    
     func (gw *Gateway) InitHostCheckManager(ctx context.Context, store storage.Handler)
    
    -func (gw *Gateway) InitializeRPCCache()
    -
     func (gw *Gateway) LoadAPI(specs ...*APISpec) (out []*APISpec)
    
     func (gw *Gateway) LoadDefinitionsFromRPCBackup() ([]*APISpec, error)
    @@ -10406,25 +10408,57 @@
     FUNCTIONS
    
     func Get() *logrus.Logger
    +    Get returns the default configured logger.
    +
     func GetRaw() *logrus.Logger
    +    GetRaw is used internally. Should likely be removed first, do not rely on
    +    it.
    +
     func LoadTranslations(thing map[string]interface{})
         LoadTranslations takes a map[string]interface and flattens it to
    -    map[string]string Because translations have been loaded - we internally
    -    override log the formatter Nested entries are accessible using dot notation.
    -    example: `{"foo": {"bar": "baz"}}` flattened: `foo.bar: baz`
    +    map[string]string. Because translations have been loaded - we internally
    +    override log the formatter. Nested entries are accessible using dot
    +    notation.
    +
    +    Example: `{"foo": {"bar": "baz"}}` Flattened: `foo.bar: baz`
    
    +func NewFormatter(format string) logrus.Formatter
    
     TYPES
    
    +type JSONFormatter struct {
    +   // TimestampFormat sets the format used for marshaling timestamps.
    +   // The format to use is the same than for time.Format or time.Parse from the standard
    +   // library.
    +   // The standard Library already provides a set of predefined format.
    +   TimestampFormat string
    +
    +   // DisableTimestamp allows disabling automatic timestamps in output.
    +   DisableTimestamp bool
    +
    +   // DataKey allows users to put all the log entry parameters into a nested dictionary at a given key.
    +   DataKey string
    +}
    +    JSONFormatter formats logs into parsable json.
    +
    +func (f *JSONFormatter) Format(entry *logrus.Entry) ([]byte, error)
    +    Format renders a single log entry
    +
     type RawFormatter struct{}
    +    RawFormatter returns the logrus entry message as bytes.
    
     func (f *RawFormatter) Format(entry *logrus.Entry) ([]byte, error)
    +    Format returns the entry.Message as a []byte.
    
     type TranslationFormatter struct {
    -   *logrus.TextFormatter
    +   logrus.Formatter
     }
    +    TranslationFormatter handles message reformatting with translations.
    
     func (t *TranslationFormatter) Format(entry *logrus.Entry) ([]byte, error)
    +    Format will translate the log message based on the message code. This is a
    +    HTTP response code if provided. The message is usually just "Finished" for
    +    those cases, this would likely produce a better log message.
    
     # Package: ./regexp
    
    github-actions[bot] commented 2 months ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 3 ๐Ÿ”ต๐Ÿ”ต๐Ÿ”ตโšชโšช
    ๐Ÿงช PR contains tests
    ๐Ÿ”’ No security concerns identified
    โšก Key issues to review

    Function Renaming
    The function `initRPCCache` was renamed from `InitializeRPCCache`. Ensure that all references to the old function name are updated across the project to avoid runtime errors. Error Handling
    The function `initSystem` logs a fatal error if it encounters an issue but does not return the error for further handling. Consider modifying the function to return errors and handle them gracefully where `initSystem` is called. Error Handling
    In the `JSONFormatter`'s `Format` method, the error from `enc.Encode(data)` is not checked before returning. It's crucial to handle this potential error to avoid data inconsistencies or crashes.
    github-actions[bot] commented 2 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for cache initialization ___ **Consider checking for errors when calling cache.New for both RPCGlobalCache and
    RPCCertCache. This will ensure that any initialization errors are handled properly
    and do not cause runtime issues.** [gateway/server.go [269-270]](https://github.com/TykTechnologies/tyk/pull/6484/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525bR269-R270) ```diff -gw.RPCGlobalCache = cache.New(int64(conf.SlaveOptions.RPCGlobalCacheExpiration), 15) -gw.RPCCertCache = cache.New(int64(conf.SlaveOptions.RPCCertCacheExpiration), 15) +var err error +gw.RPCGlobalCache, err = cache.New(int64(conf.SlaveOptions.RPCGlobalCacheExpiration), 15) +if err != nil { + return err +} +gw.RPCCertCache, err = cache.New(int64(conf.SlaveOptions.RPCCertCacheExpiration), 15) +if err != nil { + return err +} ```
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by adding error handling for cache initialization, which is crucial for preventing runtime issues and ensuring robustness.
    9
    Maintainability
    Refactor logging setup into a separate function within initSystem ___ **Refactor the initSystem function to separate concerns, specifically extracting the
    log level and format setup into a separate function. This improves readability and
    maintainability.** [gateway/server.go [1214-1239]](https://github.com/TykTechnologies/tyk/pull/6484/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525bR1214-R1239) ```diff func (gw *Gateway) initSystem() error { ... + gw.setupLogging() + ... +} + +func (gw *Gateway) setupLogging() { if os.Getenv("TYK_LOGFORMAT") == "" && !*cli.DebugMode { log.Formatter = logger.NewFormatter(gwConfig.LogFormat) mainLog.Debugf("Set log format to %q", gwConfig.LogFormat) } - ... if os.Getenv("TYK_LOGLEVEL") == "" && !*cli.DebugMode { level := strings.ToLower(gwConfig.LogLevel) switch level { case "", "info": // default, do nothing case "error": log.Level = logrus.ErrorLevel case "warn": log.Level = logrus.WarnLevel case "debug": log.Level = logrus.DebugLevel default: mainLog.Fatalf("Invalid log level %q specified in config, must be error, warn, debug or info. ", level) } mainLog.Debugf("Set log level to %q", log.Level) } - ... } ```
    Suggestion importance[1-10]: 7 Why: The refactoring suggestion enhances code readability and maintainability by separating concerns, which is beneficial for future modifications and understanding the code structure.
    7
    Replace string literals with constants for log levels ___ **Use a constant or configuration setting for the log level strings to avoid potential
    typos and to make maintenance easier.** [gateway/server.go [1227-1234]](https://github.com/TykTechnologies/tyk/pull/6484/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525bR1227-R1234) ```diff -case "error": -case "warn": -case "debug": -case "info": +case logLevelError: +case logLevelWarn: +case logLevelDebug: +case logLevelInfo: ```
    Suggestion importance[1-10]: 6 Why: Replacing string literals with constants improves maintainability and reduces the risk of typos, but it is a minor improvement and not critical to functionality.
    6
    Best practice
    Encapsulate environment variable access in a helper function ___ **Instead of using os.Getenv directly in the initSystem function, consider creating a
    helper function to encapsulate environment variable access, providing a single point
    of modification for future changes or enhancements.** [gateway/server.go [1218-1239]](https://github.com/TykTechnologies/tyk/pull/6484/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525bR1218-R1239) ```diff -if os.Getenv("TYK_LOGFORMAT") == "" && !*cli.DebugMode { +if getEnv("TYK_LOGFORMAT") == "" && !*cli.DebugMode { log.Formatter = logger.NewFormatter(gwConfig.LogFormat) mainLog.Debugf("Set log format to %q", gwConfig.LogFormat) } -if os.Getenv("TYK_LOGLEVEL") == "" && !*cli.DebugMode { +if getEnv("TYK_LOGLEVEL") == "" && !*cli.DebugMode { level := strings.ToLower(gwConfig.LogLevel) switch level { case "", "info": // default, do nothing case "error": log.Level = logrus.ErrorLevel case "warn": log.Level = logrus.WarnLevel case "debug": log.Level = logrus.DebugLevel default: mainLog.Fatalf("Invalid log level %q specified in config, must be error, warn, debug or info. ", level) } mainLog.Debugf("Set log level to %q", log.Level) } +func getEnv(key string) string { + return os.Getenv(key) +} + ```
    Suggestion importance[1-10]: 5 Why: Encapsulating environment variable access in a helper function is a good practice for maintainability, but it is a minor improvement and not essential for the current functionality.
    5
    sonarcloud[bot] commented 2 months ago

    Quality Gate Failed Quality Gate failed

    Failed conditions
    47.8% Coverage on New Code (required โ‰ฅ 80%)
    C Reliability Rating on New Code (required โ‰ฅ A)

    See analysis details on SonarCloud

    Catch issues before they fail your Quality Gate with our IDE extension SonarLint