Skip to content
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.

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

[TT-12193] Add log for event handler webhook #6310

Merged
merged 3 commits into from
May 27, 2024

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented May 27, 2024

No description provided.

Copy link
Contributor

PR Description updated to latest commit (99a4f28)

Copy link
Contributor

github-actions bot commented May 27, 2024

API Changes

--- prev.txt	2024-05-27 09:54:01.977725919 +0000
+++ current.txt	2024-05-27 09:53:58.985704414 +0000
@@ -10255,6 +10255,9 @@
 func (w *WebHookHandler) Checksum(reqBody string) (string, error)
 
 func (w *WebHookHandler) CreateBody(em config.EventMessage) (string, error)
+    CreateBody will render the webhook event message template. If an error
+    occurs, a partially rendered template will be returned alongside the error
+    that occured.
 
 func (w *WebHookHandler) HandleEvent(em config.EventMessage)
     HandleEvent will be fired when the event handler instance is found in an

Copy link
Contributor

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the changes are limited to a single function with a straightforward addition of error handling and logging. The logic is simple and the impact is localized.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The function HandleEvent does not return any value when an error occurs in CreateBody. This might be intended for logging only, but if the function is expected to return a response or further handle the error, this could be an oversight.

🔒 Security concerns

No

Code feedback:
relevant filegateway/event_handler_webhooks.go
suggestion      

Consider adding more context to the error log for better traceability. Include additional fields such as the event type or other identifiers from the EventMessage. This will help in quickly pinpointing issues in a production environment. [important]

relevant line}).Error("Webhook request failed: ", err)

Copy link
Contributor

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add error handling for the w.BuildRequest(reqBody) function to improve robustness and error logging

Consider handling the error returned by w.BuildRequest(reqBody) to ensure that failures in
request construction are properly logged and managed. This is crucial for debugging and
maintaining the robustness of the webhook handling.

gateway/event_handler_webhooks.go [224]

 req, err := w.BuildRequest(reqBody)
+if err != nil {
+    log.WithFields(logrus.Fields{
+        "prefix": "webhooks",
+    }).Error("Failed to build request: ", err)
+    return
+}
 
Suggestion importance[1-10]: 9

Why: This suggestion addresses a potential bug by adding error handling for w.BuildRequest(reqBody), which is crucial for robustness and debugging. It ensures that any errors in request construction are logged and managed properly.

9
Enhancement
Enhance the error logging by including event-specific details for better traceability

Include more specific information in the error log at line 219, such as the event message
details, to provide better context for debugging issues related to webhook requests.

gateway/event_handler_webhooks.go [217-219]

 log.WithFields(logrus.Fields{
     "prefix": "webhooks",
+    "eventID": em.ID,
+    "eventType": em.Type,
 }).Error("Webhook request failed: ", err)
 
Suggestion importance[1-10]: 8

Why: Adding event-specific details to the error log improves traceability and debugging, making it easier to identify and resolve issues related to specific events.

8
Best practice
Use fmt.Sprintf for error message formatting to ensure proper log output

To ensure that the error message is properly formatted and concatenated in the log, use
fmt.Errorf with formatted strings instead of concatenation.

gateway/event_handler_webhooks.go [219]

-}).Error("Webhook request failed: ", err)
+}).Error(fmt.Sprintf("Webhook request failed: %v", err))
 
Suggestion importance[1-10]: 7

Why: Using fmt.Sprintf for error message formatting is a best practice that ensures proper log output and avoids potential issues with string concatenation.

7

Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@titpetric titpetric changed the title Add log for event handler webhook [TT-12193Add log for event handler webhook May 27, 2024
@titpetric titpetric changed the title [TT-12193Add log for event handler webhook [TT-12193] Add log for event handler webhook May 27, 2024
Copy link

sonarcloud bot commented May 27, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
58.3% 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

@titpetric titpetric enabled auto-merge (squash) May 27, 2024 10:43
@titpetric titpetric merged commit b759583 into master May 27, 2024
35 of 36 checks passed
@titpetric titpetric deleted the fix/tt-12193/fix-create-body-error-handling branch May 27, 2024 10:54
@titpetric
Copy link
Contributor Author

/release to release-5.3

Copy link

tykbot bot commented May 27, 2024

Working on it! Note that it can take a few minutes.

Copy link

tykbot bot commented May 27, 2024

Still working...

tykbot bot pushed a commit that referenced this pull request May 27, 2024
Co-authored-by: Tit Petric <tit@tyk.io>
(cherry picked from commit b759583)
Copy link

tykbot bot commented May 27, 2024

@titpetric Succesfully merged PR

buger added a commit that referenced this pull request May 27, 2024
…6310)

[TT-12193] Add log for event handler webhook (#6310)

Co-authored-by: Tit Petric <tit@tyk.io>
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.

None yet

2 participants