Refactor kncloudevents and add `SendEvent` function #7092

creydr · 2023-07-14T12:22:29Z

Adds a function (SendEvent) to the kncloudevents package to Send events. This function:

configures the client (e.g. for TLS)
has option to send replies
has option to send to a DLS
has option to add additional transformers

This allows e.g. for use cases like the following:

kncloudevents.SendEvent(ctx, event, sub.Subscriber,
	kncloudevents.WithHeader(additionalHeaders),
	kncloudevents.WithReply(sub.Reply),
	kncloudevents.WithDeadLetterSink(sub.DeadLetter),
	kncloudevents.WithRetryConfig(sub.RetryConfig))

The SendMessage function is provided to make integration with existing packages easier (e.g. in dependent projects).

Also migrated the usages of kncloudevents.NewCloudEventRequest() to the new SendEvent() function (mt-broker-filter and mt-broker-ingress):

pkg/kncloudevents/message_dispatcher.go

codecov · 2023-07-25T13:52:41Z

Codecov Report

Patch coverage: 71.00% and project coverage change: -0.17% ⚠️

Comparison is base (874d288) 78.05% compared to head (e11f71a) 77.89%.
Report is 4 commits behind head on main.

❗ Current head e11f71a differs from pull request most recent head 6edacd9. Consider uploading reports for the commit 6edacd9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7092      +/-   ##
==========================================
- Coverage   78.05%   77.89%   -0.17%     
==========================================
  Files         250      249       -1     
  Lines       13390    13445      +55     
==========================================
+ Hits        10452    10473      +21     
- Misses       2405     2433      +28     
- Partials      533      539       +6

Files Changed	Coverage Δ
pkg/broker/filter/server_manager.go	`0.00% <0.00%> (ø)`
pkg/broker/ingress/server_manager.go	`0.00% <0.00%> (ø)`
pkg/channel/message_dispatcher.go	`74.63% <ø> (+1.46%)`	⬆️
pkg/kncloudevents/attributes/knative_error.go	`100.00% <ø> (ø)`
pkg/kncloudevents/http_client_new.go	`60.33% <ø> (-0.33%)`	⬇️
pkg/kncloudevents/message_sender_new.go	`83.78% <ø> (ø)`
pkg/kncloudevents/utils.go	`72.72% <ø> (+2.72%)`	⬆️
...reconciler/inmemorychannel/dispatcher/readiness.go	`72.41% <ø> (ø)`
pkg/utils/headers.go	`100.00% <ø> (ø)`
pkg/kncloudevents/event_dispatcher.go	`63.12% <63.12%> (ø)`
... and 9 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

creydr · 2023-07-26T07:23:18Z

/retest

creydr · 2023-07-26T12:18:32Z

/retest

creydr · 2023-07-28T09:46:39Z

Hello @pierDipi,
this PR is ready for review now. Could you PTAL? I also added the migration of the existing usages of the kncloudevents.NewCloudEventRequest() to the new SendEvent() function (could have been done in a seperate PR too, but I wanted to confirm, that everything works as expected with the new lib too).

/assign @pierDipi

pkg/broker/filter/filter_handler_test.go

pkg/channel/fanout/fanout_event_handler_test.go

creydr · 2023-07-28T10:34:07Z

/hold
need to recheck for the DLS

pkg/channel/multichannelfanout/multi_channel_fanout_event_handler_test.go

matzew · 2023-08-01T14:15:04Z

OK. cool. Let us know when this is no longer WIP, and ready for review

matzew · 2023-08-01T14:15:25Z

FWIW: I ran the test now locally as well, and passes here too - after pulling the updated branch

matzew · 2023-08-01T15:08:00Z

/test upgrade-tests

creydr · 2023-08-01T18:47:33Z

/retest

creydr · 2023-08-01T20:40:57Z

/retest

creydr · 2023-08-02T05:57:55Z

/unhold

@pierDipi @matzew could you recheck on this PR?

matzew · 2023-08-02T07:10:05Z

pkg/kncloudevents/event_dispatcher_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package channel
+package kncloudevents_test


package _test ?

yes, to prevent an import cycle. We are importing here pkg/eventingtls/eventingtlstesting for the TLS tests, which is using pkg/kncloudevents.

matzew · 2023-08-02T12:13:27Z

pkg/utils/headers.go

- // tracing
- "x-request-id",
+ "x-request-id", // tracing
+ "retry-after",


ah, nice - removing the other frm the filter

matzew

/lgtm
/approve

knative-prow · 2023-08-02T12:13:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [creydr,matzew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

matzew · 2023-08-02T12:43:27Z

/test upgrade-tests

Adds a function (`SendEvent`) to the kncloudevents package to Send events. This function: * configures the client (e.g. for TLS) * has option to send replies * has option to send to a DLS * has option to add additional transformers This allows e.g. for use cases like the following: ``` kncloudevents.SendEvent(ctx, event, sub.Subscriber, kncloudevents.WithHeader(additionalHeaders), kncloudevents.WithReply(sub.Reply), kncloudevents.WithDeadLetterSink(sub.DeadLetter), kncloudevents.WithRetryConfig(sub.RetryConfig)) ``` The `SendMessage` function is provided to make integration with existing packages easier (e.g. in dependent projects). Also migrated the usages of kncloudevents.NewCloudEventRequest() to the new SendEvent() function (mt-broker-filter and mt-broker-ingress): * 74c1552 * 9728713 * 958722d

* Remove deprecated httpclient msgsender (knative#7018) ## Proposed Changes Removed the legacy http client go, message sender go and respective test files, Since the functionality are handled in http client new and message sender new go Issue related : knative#6995 - 🗑️ Remove feature or internal logic - - - ### Pre-review Checklist - [x] **At least 80% unit test coverage** - [ ] **E2E tests** for any new behavior - [ ] **Docs PR** for any user-facing impact - [ ] **Spec PR** for any new API feature - [ ] **Conformance test** for any change to the spec **Release Note** ```release-note ``` **Docs** 📖 knative#6995 --------- Co-authored-by: Christoph Stäbler <[email protected]> * Refactor kncloudevents and add `SendEvent` function (knative#7092) Adds a function (`SendEvent`) to the kncloudevents package to Send events. This function: * configures the client (e.g. for TLS) * has option to send replies * has option to send to a DLS * has option to add additional transformers This allows e.g. for use cases like the following: ``` kncloudevents.SendEvent(ctx, event, sub.Subscriber, kncloudevents.WithHeader(additionalHeaders), kncloudevents.WithReply(sub.Reply), kncloudevents.WithDeadLetterSink(sub.DeadLetter), kncloudevents.WithRetryConfig(sub.RetryConfig)) ``` The `SendMessage` function is provided to make integration with existing packages easier (e.g. in dependent projects). Also migrated the usages of kncloudevents.NewCloudEventRequest() to the new SendEvent() function (mt-broker-filter and mt-broker-ingress): * 74c1552 * 9728713 * 958722d * Remove deprecated kncloudevents.CloudEventsRequest (knative#7146) * Remove kncloudevents.CloudEventsRequest * Run hack/update-codegen.sh * Add unit tests for generateBackoffFn() * Fix event dispatcher library data race (knative#7280) Fix dispatcher data race Signed-off-by: Pierangelo Di Pilato <[email protected]> --------- Signed-off-by: Pierangelo Di Pilato <[email protected]> Co-authored-by: Jeevan <[email protected]> Co-authored-by: Christoph Stäbler <[email protected]>

knative-prow bot requested review from aliok and matzew July 14, 2023 12:22

creydr commented Jul 14, 2023

View reviewed changes

pkg/kncloudevents/message_dispatcher.go Outdated Show resolved Hide resolved

creydr force-pushed the refactor-kncloudevents branch from aee859b to 89a57d1 Compare July 25, 2023 13:41

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2023

creydr force-pushed the refactor-kncloudevents branch from 4223034 to 3429faf Compare July 25, 2023 15:36

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2023

creydr added 8 commits July 27, 2023 10:06

Copy message dispatcher to kncloudevents package

4e10f95

Remove interfaces

d97f8c2

Rename function and change signature to use options

772a697

Refactor to include response headers in DispatchInfo

b55f5d2

Use http client wrapper

8cd8f63

Return error if destination is nil instead of sending it to reply

42d073a

Add option for additional transformers

78ae675

Add option to specify retryConfig

025a8e4

creydr force-pushed the refactor-kncloudevents branch from 82a90ac to 92ffb44 Compare July 27, 2023 08:44

creydr changed the title ~~[WIP] Refactor kncloudevents SendMessage~~ Refactor kncloudevents SendMessage Jul 28, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2023

knative-prow bot assigned pierDipi Jul 28, 2023

creydr changed the title ~~Refactor kncloudevents SendMessage~~ Refactor kncloudevents and add SendEvent function Jul 28, 2023

pierDipi reviewed Jul 28, 2023

View reviewed changes

pkg/broker/filter/filter_handler_test.go Outdated Show resolved Hide resolved

pkg/channel/fanout/fanout_event_handler_test.go Outdated Show resolved Hide resolved

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2023

Filter headers in ingress-handler

1e1203f

creydr force-pushed the refactor-kncloudevents branch from 0932a21 to 1e1203f Compare July 31, 2023 14:29

creydr added 4 commits July 31, 2023 16:33

Remove unneeded transformers param from receiverFunc

56ed455

Simplify event-receiver serveHTTP

8dc2540

Wrap errors in event_dispatcher

062d28e

Filter headers in filter-handler

64c97f1

matzew reviewed Aug 1, 2023

View reviewed changes

pkg/channel/multichannelfanout/multi_channel_fanout_event_handler_test.go Show resolved Hide resolved

creydr added 4 commits August 1, 2023 15:07

Fix filter for errorExtension response

dba0268

Move knativeErrorTransformer to kncloudevents package

0aff6d6

Simplify filter_handler ServeHTTP

e11f71a

Remove old message_dispatcher

6edacd9

creydr changed the title ~~[WIP] Refactor kncloudevents and add SendEvent function~~ Refactor kncloudevents and add SendEvent function Aug 2, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2023

matzew reviewed Aug 2, 2023

View reviewed changes

matzew approved these changes Aug 2, 2023

View reviewed changes

knative-prow bot assigned matzew Aug 2, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2023

knative-prow bot merged commit 3915df7 into knative:main Aug 2, 2023
24 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor kncloudevents and add `SendEvent` function #7092

Refactor kncloudevents and add `SendEvent` function #7092

creydr commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 25, 2023 •

edited

Loading

creydr commented Jul 26, 2023

creydr commented Jul 26, 2023

creydr commented Jul 28, 2023

creydr commented Jul 28, 2023

matzew commented Aug 1, 2023

matzew commented Aug 1, 2023

matzew commented Aug 1, 2023

creydr commented Aug 1, 2023

creydr commented Aug 1, 2023

creydr commented Aug 2, 2023

matzew Aug 2, 2023

creydr Aug 2, 2023

matzew Aug 2, 2023

matzew Aug 2, 2023

matzew left a comment

knative-prow bot commented Aug 2, 2023

matzew commented Aug 2, 2023

Refactor kncloudevents and add SendEvent function #7092

Refactor kncloudevents and add SendEvent function #7092

Conversation

creydr commented Jul 14, 2023 • edited Loading

codecov bot commented Jul 25, 2023 • edited Loading

Codecov Report

creydr commented Jul 26, 2023

creydr commented Jul 26, 2023

creydr commented Jul 28, 2023

creydr commented Jul 28, 2023

matzew commented Aug 1, 2023

matzew commented Aug 1, 2023

matzew commented Aug 1, 2023

creydr commented Aug 1, 2023

creydr commented Aug 1, 2023

creydr commented Aug 2, 2023

matzew Aug 2, 2023

Choose a reason for hiding this comment

creydr Aug 2, 2023

Choose a reason for hiding this comment

matzew Aug 2, 2023

Choose a reason for hiding this comment

matzew Aug 2, 2023

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Aug 2, 2023

matzew commented Aug 2, 2023

Refactor kncloudevents and add `SendEvent` function #7092

Refactor kncloudevents and add `SendEvent` function #7092

creydr commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 25, 2023 •

edited

Loading