Skip to content

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

Merged
titpetric merged 3 commits into
masterfrom
fix/tt-12193/fix-create-body-error-handling
May 27, 2024
Merged

[TT-12193] Add log for event handler webhook#6310
titpetric merged 3 commits into
masterfrom
fix/tt-12193/fix-create-body-error-handling

Conversation

@titpetric

@titpetric titpetric commented May 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

No description provided.

@titpetric
titpetric requested review from andrei-tyk and lghiur May 27, 2024 08:19
@github-actions

github-actions Bot commented May 27, 2024

Copy link
Copy Markdown
Contributor

PR Description updated to latest commit (99a4f28)

@github-actions

github-actions Bot commented May 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented May 27, 2024

Copy link
Copy Markdown
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)

@github-actions

github-actions Bot commented May 27, 2024

Copy link
Copy Markdown
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

@github-actions

github-actions Bot commented May 27, 2024

Copy link
Copy Markdown
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
@sonarqubecloud

sonarqubecloud Bot commented May 27, 2024

Copy link
Copy Markdown

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
@titpetric
titpetric deleted the fix/tt-12193/fix-create-body-error-handling branch May 27, 2024 10:54
@titpetric

titpetric commented May 27, 2024

Copy link
Copy Markdown
Contributor Author

/release to release-5.3

@tykbot

tykbot Bot commented May 27, 2024

Copy link
Copy Markdown

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

@tykbot

tykbot Bot commented May 27, 2024

Copy link
Copy Markdown

Still working...

tykbot Bot pushed a commit that referenced this pull request May 27, 2024
[TT-12193] Add log for event handler webhook (#6310)
b82d1a2 
Co-authored-by: Tit Petric <[email protected]>
(cherry picked from commit b759583)
@buger buger mentioned this pull request May 27, 2024
Merged
@tykbot

tykbot Bot commented May 27, 2024

Copy link
Copy Markdown

@titpetric Succesfully merged PR

buger added a commit that referenced this pull request May 27, 2024
@buger
Merging to release-5.3: [TT-12193] Add log for event handler webhook (#…
7c71e51 
…6310)

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

Co-authored-by: Tit Petric <[email protected]>
nerdydread pushed a commit that referenced this pull request Sep 6, 2024
@nerdydread
[TT-12193] Add log for event handler webhook (#6310)
fccc236 
Co-authored-by: Tit Petric <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrei-tyk andrei-tyk andrei-tyk approved these changes
@lghiur lghiur Awaiting requested review from lghiur

Assignees

No one assigned

Labels

Bug fix enhancement Review effort [1-5]: 2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@titpetric @andrei-tyk