TykTechnologies / tyk

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

[TT-13008]: modified default streams logger #6665

Open kofoworola opened 4 days ago

kofoworola commented 4 days ago

User description

TT-13008
Summary Backoff strategy for reconnection when broker is down
Type Story Story
Status In Dev
Points N/A
Labels -

Description

Added a logger to created streams to change the format of logging used by benthos TT-13008

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist


PR Type

enhancement


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
manager.go
Add logger configuration to stream creation                           

ee/middleware/streams/manager.go
  • Added a logger configuration to the stream creation process.
  • Configured logger with level 'INFO', format 'json', and timestamp
    addition.
  • Added static fields to logger configuration, including the stream ID.
  • +10/-0   

    πŸ’‘ PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    buger commented 4 days ago

    Knock Knock! πŸ”

    Just thought I'd let you know that your PR title and story title look quite different. PR titles that closely resemble the story title make it easier for reviewers to understand the context of the PR.

    An easy-to-understand PR title a day makes the reviewer review away! πŸ˜›βš‘οΈ
    Story Title Backoff strategy for reconnection when broker is down
    PR Title [TT-13008]: modified default streams logger

    Check out this guide to learn more about PR best-practices.

    github-actions[bot] commented 4 days ago

    PR Reviewer Guide πŸ”

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 πŸ”΅πŸ”΅βšͺβšͺβšͺ
    πŸ§ͺ No relevant tests
    πŸ”’ No security concerns identified
    ⚑ Recommended focus areas for review

    Configuration Hardcoding
    The logger configuration is hardcoded within the function, which might limit flexibility and configurability in different environments. Consider externalizing this configuration or providing a way to override these settings.
    github-actions[bot] commented 4 days ago

    API Changes

    no api changes detected
    github-actions[bot] commented 4 days ago

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Check and initialize config if nil before adding logger settings ___ **Ensure that the config map is not nil before attempting to add the logger
    configuration to it. This can prevent a potential nil map panic.** [ee/middleware/streams/manager.go [89-96]](https://github.com/TykTechnologies/tyk/pull/6665/files#diff-3e372b3346d8d296e6953152c89202a634d7654f10549676af9aea8628e13dfbR89-R96) ```diff +if config == nil { + config = make(map[string]interface{}) +} config["logger"] = map[string]interface{}{ "level": "INFO", "format": "json", "add_timestamp": true, "static_fields": map[string]interface{}{ "stream": streamID, }, } ```
    Suggestion importance[1-10]: 9 Why: Ensuring that the `config` map is not nil before adding to it prevents a potential nil map panic, which is a critical bug that could cause the application to crash.
    9
    Possible issue
    Validate streamID to ensure it meets expected criteria before usage ___ **Consider validating the streamID before using it to create streamFullID and adding
    it to config. This can prevent potential issues with invalid or malicious stream
    IDs.** [ee/middleware/streams/manager.go [85-94]](https://github.com/TykTechnologies/tyk/pull/6665/files#diff-3e372b3346d8d296e6953152c89202a634d7654f10549676af9aea8628e13dfbR85-R94) ```diff +if !isValidStreamID(streamID) { + return fmt.Errorf("invalid stream ID") +} streamFullID := fmt.Sprintf("%s_%s", sm.mw.Spec.APIID, streamID) ... "stream": streamID, ```
    Suggestion importance[1-10]: 8 Why: Validating `streamID` before using it can prevent potential issues with invalid or malicious inputs, which is crucial for maintaining the integrity and security of the system.
    8
    Best practice
    Replace magic strings and values in logger configuration with constants ___ **Use constants for logger configuration values like "level", "format", and
    "add_timestamp" to avoid typos and facilitate changes.** [ee/middleware/streams/manager.go [90-92]](https://github.com/TykTechnologies/tyk/pull/6665/files#diff-3e372b3346d8d296e6953152c89202a634d7654f10549676af9aea8628e13dfbR90-R92) ```diff -"level": "INFO", -"format": "json", +"level": LogLevelInfo, +"format": LogFormatJSON, "add_timestamp": true, ```
    Suggestion importance[1-10]: 6 Why: Using constants for configuration values reduces the risk of typos and makes future changes easier, promoting best practices in code management.
    6
    Maintainability
    Refactor logger configuration into a separate function for clarity and reuse ___ **Consider extracting the logger configuration into a separate function for better
    modularity and reusability.** [ee/middleware/streams/manager.go [89-96]](https://github.com/TykTechnologies/tyk/pull/6665/files#diff-3e372b3346d8d296e6953152c89202a634d7654f10549676af9aea8628e13dfbR89-R96) ```diff -config["logger"] = map[string]interface{}{ - "level": "INFO", - "format": "json", - "add_timestamp": true, - "static_fields": map[string]interface{}{ - "stream": streamID, - }, -} +config["logger"] = getLoggerConfig(streamID) ```
    Suggestion importance[1-10]: 5 Why: Extracting the logger configuration into a separate function improves code modularity and reusability, enhancing maintainability, though it is not critical for functionality.
    5