[TT-12702] revert wrappedServeHTTP to use recordDetail #6654

jeffy-mathew commented 1 month ago

User description

Summary	Regression in Gateway handling larger payloads (speed and memory usage)
Type	Bug
Status	In Test
Points	N/A
Labels	customer_bug, jira_escalated

Description

release from https://github.com/TykTechnologies/tyk/pull/6654

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

Bug fix, Tests

Description

Simplified the logic in recordDetail function to determine detailed recording based on org session and added a condition to check if GraphQL is enabled.
Updated ServeHTTP in reverse_proxy.go to use the recordDetail function for deciding on detailed recording.
Added a new test case in handler_success_test.go for handling GraphQL requests.
Introduced a benchmark test in reverse_proxy_test.go to evaluate performance with large response payloads.
Corrected the repository URL in the GitHub Actions release workflow configuration.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files

Bug fix

handler_success.go `Simplify detailed recording logic and add GraphQL check` gateway/handler_success.go Simplified logic for determining detailed recording based on org session. Added condition to check GraphQL enabled status.	+8/-11
reverse_proxy.go `Use recordDetail for detailed recording decision in ServeHTTP` gateway/reverse_proxy.go Updated `ServeHTTP` to use `recordDetail` for detailed recording decision.	+1/-1

Tests

handler_success_test.go `Add test case for GraphQL request handling` gateway/handler_success_test.go - Added a test case for GraphQL request handling.	+7/-0
reverse_proxy_test.go `Add benchmark test for large response payload handling` gateway/reverse_proxy_test.go - Added benchmark test for large response payload.	+38/-0

Configuration changes

release.yml

Update repository URL in release workflow

.github/workflows/release.yml - Updated repository URL in GitHub Actions workflow.

+1/-1

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

buger commented 1 month 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	Regression in Gateway handling larger payloads (speed and memory usage)
PR Title	[TT-12702] revert wrappedServeHTTP to use recordDetail #6654

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

github-actions[bot] commented 1 month ago

API Changes

no api changes detected

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Deprecated Feature Use
The code uses deprecated session detail recording flags which might be removed in future releases. Consider refactoring or replacing this deprecated functionality. Logic Change
The logic for enabling detailed recording has been changed significantly. Ensure that these changes align with intended business logic and do not introduce regressions.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Prevent potential panics by checking for nil before type assertion ___ Consider checking the result of `r.Context().Value(ctx.OrgSessionContext)` for nil before type assertion to avoid potential panics if the context value is not set. [gateway/handler_success.go [344]](https://github.com/TykTechnologies/tyk/pull/6683/files#diff-45135957493eca37f2e3e9a81079577777133c53b27cf95ea2ff0329c05bd006R344-R344) ```diff -session, ok := r.Context().Value(ctx.OrgSessionContext).(user.SessionState) +sessionValue := r.Context().Value(ctx.OrgSessionContext) +if sessionValue == nil { + return false +} +session, ok := sessionValue.(user.SessionState) ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential panic by checking for nil before performing a type assertion, which is crucial for preventing runtime errors and improving the robustness of the code.	9
Possible bug	Improve robustness by handling failed type assertions ___ Add error handling for the `r.Context().Value(ctx.OrgSessionContext)` type assertion to handle the case where the assertion fails. [gateway/handler_success.go [344-345]](https://github.com/TykTechnologies/tyk/pull/6683/files#diff-45135957493eca37f2e3e9a81079577777133c53b27cf95ea2ff0329c05bd006R344-R345) ```diff session, ok := r.Context().Value(ctx.OrgSessionContext).(*user.SessionState) -if ok && session != nil { +if !ok { + return false // or handle the error appropriately +} +if session != nil { ``` Suggestion importance[1-10]: 8 Why: The suggestion enhances the robustness of the code by handling cases where the type assertion fails, which is important for avoiding unexpected behavior and ensuring the function returns a sensible default.	8
Maintainability	Enhance code clarity and maintainability by separating conditional checks ___ Refactor the condition to check `session.EnableDetailedRecording` and `session.EnableDetailRecording` separately for clearer logic and easier future modifications. [gateway/handler_success.go [346]](https://github.com/TykTechnologies/tyk/pull/6683/files#diff-45135957493eca37f2e3e9a81079577777133c53b27cf95ea2ff0329c05bd006R346-R346) ```diff -return session.EnableDetailedRecording \|\| session.EnableDetailRecording +if session.EnableDetailedRecording { + return true +} +return session.EnableDetailRecording ``` Suggestion importance[1-10]: 5 Why: While the suggestion improves readability by separating conditional checks, the functional impact is minimal since the logic remains unchanged. It mainly aids in future code modifications.	5
Maintainability	Improve code consistency and readability by explicitly handling conditions ___ Ensure consistency in handling the return value when no session is found, by explicitly checking the condition of `spec.GraphQL.Enabled` before returning. [gateway/handler_success.go [351]](https://github.com/TykTechnologies/tyk/pull/6683/files#diff-45135957493eca37f2e3e9a81079577777133c53b27cf95ea2ff0329c05bd006R351-R351) ```diff -return spec.GraphQL.Enabled \|\| spec.GlobalConfig.AnalyticsConfig.EnableDetailedRecording +if spec.GraphQL.Enabled { + return true +} +return spec.GlobalConfig.AnalyticsConfig.EnableDetailedRecording ``` Suggestion importance[1-10]: 5 Why: This suggestion improves readability and consistency by explicitly handling conditions, but it does not change the logic or functionality of the code, making its impact more stylistic than functional.	5