feat(webhooks): Refonte V2 Webhooks #1595

agourdel · 2024-07-04T13:25:13Z

ENG-989

coderabbitai · 2024-07-04T13:25:32Z

Important

Review skipped

More than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review.

139 files out of 198 files are above the max files limit of 50. Please upgrade to Pro plan to get higher limits.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

gfyrag · 2024-07-04T13:52:14Z

ee/webhooks/internal/commons/attempt.go

+
+func SetNextRetry(a *Attempt) {
+	now := time.Now()
+	a.NextTry = now.Add(time.Duration(1<<a.NbTry) * time.Second)


Maybe set a max ? After some try, the delay will be very long.

Also, maybe, we should allow to not have a max number of attempts.

Effectivement mais deux écoles :
On peut dire que comme le nombre de tentative max est géré par l'application, (par un paramètre) forcément le temps d'attente maximum entre deux attempts sera borné.
Ou rajouter ici un max delay.

MAIS, je sens venir gros comme une maison que le max delay choisi sera jamais le bon....

ee/webhooks/cmd/fx-modules/fx-modules.go

gfyrag · 2024-07-04T14:05:51Z

ee/webhooks/cmd/fx-modules/fx-modules.go

+			if len(r.URL.Query()) == 0 {
+				return str
+			}
+			return fmt.Sprintf("%s?%s", str, r.URL.Query().Encode())


Same for query params. Worse then :D
You should use resource attributes to store those kind of data.

This is code coming from V1.
I just used it as if because I Thought it was legacy.

As i remember, i've done the same on membership, r.URL.Path return the templated string "/test/{id}". It can be easily checked with Jeager

gfyrag · 2024-07-04T14:07:56Z

ee/webhooks/cmd/webhook-all-in-one.go

+				return serverParams
+			},
+
+			func(db *bun.DB) *storage.PostgresStore {


In Go, you can use directly some things like fx.Provide(storage.NewPostgresStoreProvider) instead of wrapping the call.

gfyrag · 2024-07-04T14:19:10Z

libs/go-libs/sync/queue.go

+func (*noCopy) Unlock() {}
+
+type Queue struct {
+	noCopy noCopy


Why are you doing that? I'm curious

Do you mean the Queue struct ?
Or the noCopy ?

gfyrag · 2024-07-04T14:20:21Z

libs/go-libs/sync/shared/sharedarr.go

+	}
+
+	if idx == 0 {
+		*s.Val = (*s.Val)[1:]


I think this will fail if the array has a size of 1 (I have a doubt)

In fact it works with Size == 1 but it failed with Size == 0
But Size == 0 is not a possible case here.

gfyrag · 2024-07-04T14:24:56Z

libs/go-libs/sync/shared/sharedmaparr.go

+package shared
+
+type SharedMapArr[T any] struct {
+	Shared[map[string]*SharedArr[T]]


I'm not against generalizing some kind of utility around object sharing, but this start to sound like very specific to a specific use case ^^

gitguardian · 2024-07-05T16:01:35Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

Dav-14 · 2024-07-08T08:23:04Z

tests/integration/suite/webhooks-ledger-committed-transaction-worker.go

 		BeforeEach(func() {
+			time.Sleep(1*time.Second)


We should avoid this in tests. Tests should not depend on time.

E.G You could implement a Ticker with manual declencher

This is orchestration tests, I need let the caches of the worker and collector runner refresh.
Whether with a sleep() or a ticker, I have to wait at least one second to be able to trigger the rest.
(And ticker is wrapper for timer by the way, with a sleep() inside)

So I think I do not understand how to do it without wait.

paul-nicolas · 2024-07-08T07:00:29Z

libs/go-libs/bun/bunmigrate/command.go

 	// Import the postgres driver.
-	_ "github.com/lib/pq"


Does this will have an impact on other modules ? Does Ledger need PQ maybe ?

Well, All the integrations tests (for each modules) are working.
I don't remember why I had to delete this line, because it goes back a long way (I discussed with Gfyrag about it)
But I think I was able to delete it because it's not used.

paul-nicolas · 2024-07-08T07:02:46Z

ee/webhooks/internal/components/webhook_collector/collector.go

@@ -0,0 +1,186 @@
+package webhookcollector


Questions here for the team:
Is it better to have internals related to each command in their own internal sub package in the cmd directory instead of here ?

That way, each command only have access to its internal codes and not the other ones ?

paul-nicolas · 2024-07-08T07:08:56Z

ee/webhooks/internal/components/webhook_controller/controllers/v1/controllers-hook.go

@@ -0,0 +1,392 @@
+package controllers


This file is a bit long and difficult to read or find the endpoint we want, I would have separate the different endpoints into different files, what do you think ?

paul-nicolas · 2024-07-08T07:15:51Z

ee/webhooks/internal/services/httpserver/default_server.go

+		})
+	})
+	defaultServer.handler.Group(func(r chi.Router) {
+		r.Get(HealthcheckRoute, func(_ http.ResponseWriter, r *http.Request) {


Healtcheck should not be under authentication and otel I think

paul-nicolas · 2024-07-08T07:17:32Z

ee/webhooks/internal/components/webhook_controller/controllers/v2/controllers-attempts.go

+
+func RegisterV2AttemptControllers(serverHttp serverInterfaces.IHTTPServer, database storeInterface.IStoreProvider) {
+
+	serverHttp.Register(string(r.V2GetWaitingAttempts.Method), r.V2GetWaitingAttempts.Url, func(w http.ResponseWriter, r *http.Request) {


Same here, having endpoints function registered in another function makes everything hard to read, but to discuss with the team

paul-nicolas · 2024-07-08T08:52:21Z

ee/webhooks/internal/utils/telemetry/log.go

@@ -0,0 +1 @@
+package telemetry


Missing code ?

paul-nicolas · 2024-07-08T08:52:51Z

ee/webhooks/internal/tests/utils.go

+
+func fillDBWithData(db storage.PostgresStore, nbHooks int, nbAttempts int) error {
+
+	// hooks := make([]*commons.Hook, 0)


paul-nicolas · 2024-07-08T08:54:48Z

ee/webhooks/internal/tests/components/webhook_controller/v2_controllers_hook_test.go

@@ -0,0 +1,228 @@
+package webhookcontroller


Question for the team: in every other services, we putted test files in the same package than the actual code ? Do we want to enforce this rule ?

paul-nicolas · 2024-07-08T08:56:25Z

ee/webhooks/internal/services/storage/postgres/postgres.go

+				for _, log := range logs {
+					event, err := commons.Event{}.FromPayload(log.Payload)
+					if err != nil {
+						//TODO(LOG) QUOI FAIRE ?


Maybe we can log it, and maybe we can add some opentelemetry metrics to add alarms ?

paul-nicolas · 2024-07-08T09:08:36Z

ee/webhooks/internal/components/webhook_controller/controllers/utils/utils.go

+)
+
+func DecodeJSONBody(r *http.Request, v any) error {
+	data, err := io.ReadAll(r.Body)


Quick opti, you can use json.NewDecoder(r.Body).Decode, instead of io.readall and unmarshall. It has its own buffering and for big body, it's actually better in performances than to raed everything at once in memory

paul-nicolas · 2024-07-08T09:11:53Z

Should we also add some code in the operator to deploy webhooks ?

Dav-14 · 2024-07-08T08:41:04Z

ee/webhooks/openapi.yaml

+              $ref: '#/components/schemas/V2HookBodyParams'
+        required: true
+      responses:
+        "200":


Maybe 201 when creating something ?

Webhook V2

96a391c

agourdel requested a review from a team as a code owner July 4, 2024 13:25

github-actions bot added the ee/webhooks label Jul 4, 2024

gfyrag reviewed Jul 4, 2024

View reviewed changes

ee/webhooks/cmd/fx-modules/fx-modules.go Outdated Show resolved Hide resolved

gfyrag reviewed Jul 4, 2024

View reviewed changes

Add maxRetry limit in handleNextRetry

1f251d8

gfyrag reviewed Jul 4, 2024

View reviewed changes

Add V2 Hooks and V1 Configs New integrations test

6a07021

agourdel force-pushed the eng-989-refacto-webhooks branch from f17da90 to 6a07021 Compare July 6, 2024 05:03

agourdel added 3 commits July 6, 2024 10:49

feat(webhooks): V2 : Last Integration test about configs and Hooks

a3ce971

Integration test for Collector

1d9999a

Add Attempts get abort Test integrations

148fdb1

agourdel force-pushed the eng-989-refacto-webhooks branch from c0b36b8 to 148fdb1 Compare July 8, 2024 06:57

Dav-14 reviewed Jul 8, 2024

View reviewed changes

paul-nicolas reviewed Jul 8, 2024

View reviewed changes

Dav-14 reviewed Jul 8, 2024

View reviewed changes

agourdel added 2 commits July 8, 2024 16:29

Fix some comments

d4f1976

Paul comment

42af0e6


		func RegisterV2AttemptControllers(serverHttp serverInterfaces.IHTTPServer, database storeInterface.IStoreProvider) {

		serverHttp.Register(string(r.V2GetWaitingAttempts.Method), r.V2GetWaitingAttempts.Url, func(w http.ResponseWriter, r *http.Request) {


		func fillDBWithData(db storage.PostgresStore, nbHooks int, nbAttempts int) error {

		// hooks := make([]*commons.Hook, 0)

feat(webhooks): Refonte V2 Webhooks #1595

Are you sure you want to change the base?

feat(webhooks): Refonte V2 Webhooks #1595

Conversation

agourdel commented Jul 4, 2024

coderabbitai bot commented Jul 4, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented Jul 5, 2024 • edited Loading

️✅ There are no secrets present in this pull request anymore.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-nicolas commented Jul 8, 2024

Choose a reason for hiding this comment

coderabbitai bot commented Jul 4, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

gitguardian bot commented Jul 5, 2024 •

edited

Loading