Skip to content

[TT-11914/TT-12101]Add OAS trafficLogs#6287

Merged
jeffy-mathew merged 4 commits into
masterfrom
feat/TT-11914/TT-12101/api-analytics-enabled
May 16, 2024
Merged

[TT-11914/TT-12101]Add OAS trafficLogs#6287
jeffy-mathew merged 4 commits into
masterfrom
feat/TT-11914/TT-12101/api-analytics-enabled

Conversation

@jeffy-mathew

@jeffy-mathew jeffy-mathew commented May 16, 2024

Copy link
Copy Markdown
Contributor

Description

Add OAS trafficLogs
Traffic logs would be enabled by default during OAS API import.
During create, it would respect what it is specified in the API definition.

Related Issue

Parent: https://tyktech.atlassian.net/browse/TT-11914
Subtask: https://tyktech.atlassian.net/browse/TT-12101

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Enhancement


Description

  • Added functionality to enable traffic logs by default during OAS API import.
  • Introduced TrafficLogs struct in middleware to handle API level log analytics.
  • Updated JSON schema to support traffic logs configuration.
  • Updated unit tests to ensure traffic logs are enabled by default.

Changes walkthrough 📝

Relevant files
Enhancement
migration.go
Set DoNotTrack flag to true by default                                     

apidef/migration.go

  • Added DoNotTrack flag set to true in APIDefinition struct.
+2/-0     
default.go
Enable traffic logs by default on OAS import                         

apidef/oas/default.go

  • Added a new function call to enable traffic logs by default during OAS
    API import.
  • +1/-0     
    middleware.go
    Implement traffic log analytics configuration                       

    apidef/oas/middleware.go

  • Added TrafficLogs struct to handle API level log analytics.
  • Implemented methods to fill and extract traffic log settings from
    APIDefinition.
  • +29/-0   
    root.go
    Conditionally enable traffic logs in middleware                   

    apidef/oas/root.go

  • Added method to conditionally enable traffic logs if they are not
    already configured.
  • +18/-0   
    Tests
    default_test.go
    Update tests to verify traffic logs are enabled                   

    apidef/oas/default_test.go

  • Updated unit tests to check for enabled traffic logs in the middleware
    configuration.
  • +66/-4   
    Configuration changes
    x-tyk-api-gateway.json
    Update JSON schema for traffic logs configuration               

    apidef/oas/schema/x-tyk-api-gateway.json

    • Updated JSON schema to include trafficLogs configuration.
    +14/-0   

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

    @github-actions

    github-actions Bot commented May 16, 2024

    Copy link
    Copy Markdown
    Contributor

    API Changes

    --- prev.txt	2024-05-16 14:50:53.873857068 +0000
    +++ current.txt	2024-05-16 14:50:50.825827438 +0000
    @@ -3157,6 +3157,9 @@
     
     	// ContextVariables contains the configuration related to Tyk context variables.
     	ContextVariables *ContextVariables `bson:"contextVariables,omitempty" json:"contextVariables,omitempty"`
    +
    +	// TrafficLogs contains the configurations related to API level log analytics.
    +	TrafficLogs *TrafficLogs `bson:"trafficLogs,omitempty" json:"trafficLogs,omitempty"`
     }
         Global contains configuration that affects the whole API (all endpoints).
     
    @@ -4216,6 +4219,19 @@
     func (i *TrackEndpoint) Fill(meta apidef.TrackEndpointMeta)
         Fill fills *TrackEndpoint receiver with data from apidef.TrackEndpointMeta.
     
    +type TrafficLogs struct {
    +	// Enabled enables traffic log analytics for the API.
    +	// Tyk classic API definition: `do_not_track`.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +}
    +    TrafficLogs holds configuration about API log analytics.
    +
    +func (t *TrafficLogs) ExtractTo(api *apidef.APIDefinition)
    +    ExtractTo extracts *TrafficLogs into *apidef.APIDefinition.
    +
    +func (t *TrafficLogs) Fill(api apidef.APIDefinition)
    +    Fill fills *TrafficLogs from apidef.APIDefinition.
    +
     type TransformBody struct {
     	// Enabled activates transform request/request body middleware.
     	Enabled bool `bson:"enabled" json:"enabled"`

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (ca91838)

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple changes across various files including Go and JSON schema updates. The logic related to enabling traffic logs and its interaction with existing API definitions needs careful review to ensure it integrates smoothly without introducing bugs.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The implementation of enableTrafficLogsIfEmpty might enable traffic logs unintentionally in scenarios where they should not be enabled, as it only checks for null and not for an explicit disable.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileapidef/oas/root.go
    suggestion      

    Consider checking if TrafficLogs.Enabled is explicitly set to false before enabling it in enableTrafficLogsIfEmpty. This ensures that explicit user configurations are respected. [important]

    relevant linex.Middleware.Global.TrafficLogs = &TrafficLogs{

    relevant fileapidef/oas/middleware.go
    suggestion      

    Add error handling for Fill and ExtractTo methods in TrafficLogs to manage potential issues during the data transformation process. This can help in identifying and debugging issues related to traffic log configuration more effectively. [important]

    relevant linefunc (t *TrafficLogs) Fill(api apidef.APIDefinition) {

    relevant fileapidef/oas/default.go
    suggestion      

    Ensure that enableTrafficLogsIfEmpty is only called when necessary to avoid redundant operations which could impact performance, especially during large imports or updates. [medium]

    relevant linexTykAPIGateway.enableTrafficLogsIfEmpty()

    relevant fileapidef/oas/middleware.go
    suggestion      

    Implement a method to toggle traffic logs dynamically based on API usage patterns or admin configurations, enhancing flexibility and control over logging features. [medium]

    relevant lineEnabled bool `bson:"enabled" json:"enabled"`

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add nil check for xTykAPIGateway to prevent nil pointer dereference

    Consider checking if xTykAPIGateway is nil before accessing its properties to avoid
    potential nil pointer dereferences.

    apidef/oas/default.go [68]

    -xTykAPIGateway.enableTrafficLogsIfEmpty()
    +if xTykAPIGateway != nil {
    +    xTykAPIGateway.enableTrafficLogsIfEmpty()
    +}
     
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a potential nil pointer dereference, which is a critical issue that could cause runtime panics.

    8
    Prevent potential nil pointer dereference by checking if api is nil

    Add a check to ensure api is not nil in the Fill method of TrafficLogs to prevent nil
    pointer dereference.

    apidef/oas/middleware.go [1503]

    -t.Enabled = !api.DoNotTrack
    +if api != nil {
    +    t.Enabled = !api.DoNotTrack
    +}
     
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical bug by adding a nil check for api in the Fill method, preventing possible runtime errors.

    8
    Possible issue
    Verify and conditionally set DoNotTrack to align with application tracking policies

    Ensure that setting DoNotTrack to true is intentional and consistent with the desired
    application behavior, as it may disable tracking unexpectedly.

    apidef/migration.go [462]

    -a.DoNotTrack = true
    +a.DoNotTrack = shouldDisableTracking()
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly points out the need to ensure that the DoNotTrack setting aligns with the intended application behavior, which is important for consistent application functionality.

    7
    Enhancement
    Allow configurable default state for TrafficLogs

    Instead of setting TrafficLogs to enabled by default, consider allowing configuration to
    dictate the default state or ensure it aligns with system-wide logging policies.

    apidef/oas/root.go [243-245]

     x.Middleware.Global.TrafficLogs = &TrafficLogs{
    -    Enabled: true,
    +    Enabled: getDefaultTrafficLogSetting(),
     }
     
    Suggestion importance[1-10]: 6

    Why: This suggestion enhances the flexibility and configurability of the TrafficLogs feature, aligning it with potential system-wide policies.

    6

    @jeffy-mathew
    jeffy-mathew force-pushed the feat/TT-11914/TT-12101/api-analytics-enabled branch 2 times, most recently from ca983f8 to d1fc5b0 Compare May 16, 2024 13:17
    @TykTechnologies TykTechnologies deleted a comment from github-actions Bot May 16, 2024
    @TykTechnologies TykTechnologies deleted a comment from github-actions Bot May 16, 2024
    @TykTechnologies TykTechnologies deleted a comment from github-actions Bot May 16, 2024
    @TykTechnologies TykTechnologies deleted a comment from github-actions Bot May 16, 2024
    @TykTechnologies TykTechnologies deleted a comment from github-actions Bot May 16, 2024
    @jeffy-mathew
    jeffy-mathew force-pushed the feat/TT-11914/TT-12101/api-analytics-enabled branch from 7e51f60 to b02f2c5 Compare May 16, 2024 13:52
    @jeffy-mathew
    jeffy-mathew force-pushed the feat/TT-11914/TT-12101/api-analytics-enabled branch from b02f2c5 to 8d43a3f Compare May 16, 2024 14:50
    @jeffy-mathew
    jeffy-mathew enabled auto-merge (squash) May 16, 2024 14:50
    @sonarqubecloud

    Copy link
    Copy Markdown

    @jeffy-mathew
    jeffy-mathew merged commit c836026 into master May 16, 2024
    @jeffy-mathew
    jeffy-mathew deleted the feat/TT-11914/TT-12101/api-analytics-enabled branch May 16, 2024 15:11
    nerdydread pushed a commit that referenced this pull request Sep 6, 2024
    ## Description
    
    Add OAS trafficLogs
    Traffic logs would be enabled by default during OAS API import.
    During create, it would respect what it is specified in the API
    definition.
    ## Related Issue
    Parent: https://tyktech.atlassian.net/browse/TT-11914
    Subtask: https://tyktech.atlassian.net/browse/TT-12101
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [ ] Bug fix (non-breaking change which fixes an issue)
    - [x] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ### **PR Type**
    Enhancement
    
    
    ___
    
    ### **Description**
    - Added functionality to enable traffic logs by default during OAS API
    import.
    - Introduced `TrafficLogs` struct in middleware to handle API level log
    analytics.
    - Updated JSON schema to support traffic logs configuration.
    - Updated unit tests to ensure traffic logs are enabled by default.
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>migration.go</strong><dd><code>Set DoNotTrack flag to
    true by default</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/migration.go
    - Added `DoNotTrack` flag set to true in `APIDefinition` struct.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6287/files#diff-e1d9b55a26f9d6225d56d6f0161959217308e5ad4d6934e7d7df4595d9c2a130">+2/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    
    <tr>
      <td>
        <details>
    <summary><strong>default.go</strong><dd><code>Enable traffic logs by
    default on OAS import</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/default.go
    <li>Added a new function call to enable traffic logs by default during
    OAS <br>API import.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6287/files#diff-83c3a85bdd05785178ee519b05b1fe2008435dc4ae9448d72b080b5f67c491ad">+1/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    
    <tr>
      <td>
        <details>
    <summary><strong>middleware.go</strong><dd><code>Implement traffic log
    analytics configuration</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/middleware.go
    <li>Added <code>TrafficLogs</code> struct to handle API level log
    analytics.<br> <li> Implemented methods to fill and extract traffic log
    settings from <br><code>APIDefinition</code>.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6287/files#diff-992ec7c28d25fd54f6491d295389757705cd114bc869a35cba50d42e548cdc6e">+29/-0</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    
    <tr>
      <td>
        <details>
    <summary><strong>root.go</strong><dd><code>Conditionally enable traffic
    logs in middleware</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/root.go
    <li>Added method to conditionally enable traffic logs if they are not
    <br>already configured.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6287/files#diff-9c56b2bdb992e0a7db76809d4c516e1cd61c9486c7f0437b344c0032476af80f">+18/-0</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr><tr><td><strong>Tests</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>default_test.go</strong><dd><code>Update tests to
    verify traffic logs are enabled</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/default_test.go
    <li>Updated unit tests to check for enabled traffic logs in the
    middleware <br>configuration.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6287/files#diff-ab6848f71731083885a9d7d7970faa68a6783a98477c78413ae3979cb5add7db">+66/-4</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr><tr><td><strong>Configuration
    changes</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>x-tyk-api-gateway.json</strong><dd><code>Update JSON
    schema for traffic logs configuration</code>&nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/schema/x-tyk-api-gateway.json
    - Updated JSON schema to include `trafficLogs` configuration.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6287/files#diff-78828969c0c04cc1a776dfc93a8bad3c499a8c83e6169f83e96d090bed3e7dd0">+14/-0</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants