Knative error extensions #2374

egorlepa · 2022-07-04T16:33:49Z

Release Note

Failed events are now decorated with Knative error extensions when routed to the `deadLetterSink` for more context

linux-foundation-easycla · 2022-07-04T16:33:56Z

The committers listed above are authorized under a signed CLA.

✅ login: egorlepa (ff6e94c, f6ff42c, f31d669)

knative-prow · 2022-07-04T16:34:03Z

Welcome @egorlepa! It looks like this is your first PR to knative-sandbox/eventing-kafka-broker 🎉

knative-prow · 2022-07-04T16:34:03Z

Hi @egorlepa. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pierDipi · 2022-07-04T16:36:56Z

/ok-to-test

codecov · 2022-07-04T16:42:27Z

Codecov Report

Merging #2374 (01ca092) into main (92a73d3) will increase coverage by 0.10%.
The diff coverage is 91.89%.

@@             Coverage Diff              @@
##               main    #2374      +/-   ##
============================================
+ Coverage     62.31%   62.42%   +0.10%     
- Complexity      708      714       +6     
============================================
  Files           146      146              
  Lines         10100    10132      +32     
  Branches        225      229       +4     
============================================
+ Hits           6294     6325      +31     
- Misses         3373     3376       +3     
+ Partials        433      431       -2

Flag	Coverage Δ
java-unittests	`81.24% <91.89%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...a/broker/dispatcher/impl/RecordDispatcherImpl.java	`89.25% <91.89%> (+1.34%)`	⬆️
...ive/eventing/kafka/broker/core/AsyncCloseable.java	`66.66% <0.00%> (-13.34%)`	⬇️
...dispatcher/impl/consumer/BaseConsumerVerticle.java	`87.09% <0.00%> (-6.46%)`	⬇️
...broker/dispatcher/impl/consumer/OffsetManager.java	`93.54% <0.00%> (-2.16%)`	⬇️
...lane/pkg/reconciler/consumergroup/consumergroup.go	`71.95% <0.00%> (+1.10%)`	⬆️
...patcher/impl/consumer/OrderedConsumerVerticle.java	`75.23% <0.00%> (+1.90%)`	⬆️
.../kafka/broker/dispatcher/impl/ResourceContext.java	`100.00% <0.00%> (+9.09%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

matzew · 2022-07-04T18:33:32Z

@egorlepa thanks for putting this PR together, I looks nice!

I will review this tomorrow. Glad to see your contribution here 🎉

egorlepa · 2022-07-04T21:15:02Z

@matzew ok, cool. i'll fix e2es tomorrow

matzew · 2022-07-05T07:44:08Z

...er/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java

-      .onSuccess(v -> onDeadLetterSinkSuccess(recordContext, finalProm))
-      .onFailure(ex -> onDeadLetterSinkFailure(recordContext, ex, finalProm));
+    // enhance event with extension attributes prior to forwarding to the dead letter sink
+    ConsumerRecordContext transformedRecordContext = errorTransform(recordContext, response);


I'd prefer using final as much as possible, to express objects are not mutable.

See other classes for examples. That would be really my only "nit" comment on the code 😄

ok, no problem. i mainly use golang and kotlin nowadays, almost forgot how to talk in java :) kotlin has nice val/var for this purpose

matzew · 2022-07-05T08:18:29Z

@egorlepa it would be nice if we had a little e2e test for this in here: https://github.com/knative-sandbox/eventing-kafka-broker/tree/main/test/e2e_new

pierDipi · 2022-07-06T10:02:02Z

/test unit-tests_eventing-kafka-broker_main

matzew · 2022-07-06T10:25:02Z

/test unit-tests_eventing-kafka-broker_main

matzew · 2022-07-06T11:12:59Z

test/e2e_new/dls_extenisons_test.go

@@ -0,0 +1,169 @@
+//go:build e2e
+// +build e2e


dls_extenisons_test.go: there is a little typo in extensions

matzew · 2022-07-06T12:12:57Z

/retest

pierDipi · 2022-07-06T12:42:21Z

test/e2e_new/dls_extensions_test.go

+				fmt.Println("dead letter sink is not addressable", err)
+			}
+			fmt.Println("dead letter sink address: ", dlsAddress.Path)
+			return cetest.HasExtension("knativeerrordest", dlsAddress.Path)


we need to include the scheme, etc

Suggested change

return cetest.HasExtension("knativeerrordest", dlsAddress.Path)

return cetest.HasExtension("knativeerrordest", dlsAddress.String())

pierDipi · 2022-07-07T16:39:15Z

test/e2e_new/dls_extensions_test.go

+	))
+	f.Setup("trigger is ready", trigger.IsReady(triggerName))
+
+	f.Setup("install source", eventshub.Install(


Suggested change

f.Setup("install source", eventshub.Install(

f.Requirement("install source", eventshub.Install(

This will make sure that we send the event only after the Broker is ready because every step at the same phase (for example, Setup) will run in parallel.

pierDipi · 2022-07-07T16:39:37Z

test/e2e_new/dls_extensions_test.go

+	))
+	f.Setup("trigger is ready", trigger.IsReady(triggerName))
+
+	f.Setup("install source", eventshub.Install(


Suggested change

f.Setup("install source", eventshub.Install(

f.Requirement("install source", eventshub.Install(

Same reasoning.

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

matzew

/lgtm
/approve

matzew · 2022-08-23T06:22:40Z

/retest

matzew · 2022-08-23T06:22:47Z

/retest-required

matzew · 2022-08-23T06:32:37Z

/test reconciler-tests_eventing-kafka-broker_main

matzew · 2022-08-23T06:32:48Z

/test reconciler-tests-namespaced-broker_eventing-kafka-broker_main

matzew · 2022-08-23T06:32:59Z

/test integration-tests-namespaced-broker_eventing-kafka-broker_main

matzew · 2022-08-23T06:33:11Z

/test integration-tests_eventing-kafka-broker_main

matzew · 2022-08-23T07:27:12Z

/test reconciler-tests_eventing-kafka-broker_main

matzew · 2022-08-23T07:27:22Z

/test reconciler-tests-namespaced-broker_eventing-kafka-broker_main

matzew · 2022-08-23T09:40:27Z

/test integration-tests-namespaced-broker_eventing-kafka-broker_main

matzew · 2022-08-23T09:43:19Z

/retest

matzew

/lgtm
/approve

knative-prow · 2022-08-23T09:53:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: egorlepa, 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~~ [matzew]

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

matzew · 2022-08-23T11:50:37Z

/cherry-pick release-1.6

matzew · 2022-08-23T11:50:43Z

/cherry-pick release-1.5

knative-prow-robot · 2022-08-23T11:51:15Z

@matzew: new pull request created: #2547

In response to this:

/cherry-pick release-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2022-08-23T11:51:19Z

@matzew: #2374 failed to apply on top of branch "release-1.5":

Applying: enhancing failed events with knativeerror* extensions before sending to dls
Applying: enhancing failed events with knativeerror* extensions before sending to dls
Using index info to reconstruct a base tree...
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
M	data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherTest.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherTest.java
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
CONFLICT (content): Merge conflict in data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 enhancing failed events with knativeerror* extensions before sending to dls
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

* enhancing failed events with knativeerror* extensions before sending to dls * enhancing failed events with knativeerror* extensions before sending to dls * fix star import * final vars & e2e draft * e2es * upgrade reconciler-test package, fix e2es * fix e2es * fix e2es * fix e2es * fix e2es * fix eventshub.SendMultipleEvents call * fix time import * More import fixes Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Populating response from ResponseFailureException Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Fixing tests Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

* Knative error extensions (knative-extensions#2374) * enhancing failed events with knativeerror* extensions before sending to dls * enhancing failed events with knativeerror* extensions before sending to dls * fix star import * final vars & e2e draft * e2es * upgrade reconciler-test package, fix e2es * fix e2es * fix e2es * fix e2es * fix e2es * fix eventshub.SendMultipleEvents call * fix time import * More import fixes Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Populating response from ResponseFailureException Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Fixing tests Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> * Remove due to incorrect deps Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: egorlepa <76429849+egorlepa@users.noreply.github.com>

* enhancing failed events with knativeerror* extensions before sending to dls * enhancing failed events with knativeerror* extensions before sending to dls * fix star import * final vars & e2e draft * e2es * upgrade reconciler-test package, fix e2es * fix e2es * fix e2es * fix e2es * fix e2es * fix eventshub.SendMultipleEvents call * fix time import * More import fixes Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Populating response from ResponseFailureException Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Fixing tests Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

* Knative error extensions (knative-extensions#2374) * enhancing failed events with knativeerror* extensions before sending to dls * enhancing failed events with knativeerror* extensions before sending to dls * fix star import * final vars & e2e draft * e2es * upgrade reconciler-test package, fix e2es * fix e2es * fix e2es * fix e2es * fix e2es * fix eventshub.SendMultipleEvents call * fix time import * More import fixes Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Populating response from ResponseFailureException Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Fixing tests Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> * Remove due to incorrect deps Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: egorlepa <76429849+egorlepa@users.noreply.github.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 4, 2022

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/data-plane labels Jul 4, 2022

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 4, 2022

egorlepa changed the title ~~Knative error extensions~~ [WIP] Knative error extensions Jul 4, 2022

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

matzew reviewed Jul 5, 2022

View reviewed changes

danielezonca mentioned this pull request Jul 5, 2022

Enrich failed events with custom attributes provided via HTTP Response Headers #2376

Closed

knative-prow bot added the area/test label Jul 6, 2022

pierDipi mentioned this pull request Jul 6, 2022

[eventshub] Allow defining a custom response body for skipped events knative-extensions/reconciler-test#358

Merged

matzew reviewed Jul 6, 2022

View reviewed changes

pierDipi reviewed Jul 6, 2022

View reviewed changes

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

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2022

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

pierDipi reviewed Jul 7, 2022

View reviewed changes

Fixing tests

01ca092

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

matzew changed the title ~~[WIP] Knative error extensions~~ Knative error extensions Aug 23, 2022

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 23, 2022

matzew approved these changes Aug 23, 2022

View reviewed changes

knative-prow bot assigned matzew Aug 23, 2022

knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 23, 2022

matzew approved these changes Aug 23, 2022

View reviewed changes

knative-prow bot merged commit 401a877 into knative-extensions:main Aug 23, 2022

knative-prow-robot mentioned this pull request Aug 23, 2022

[release-1.6] Knative error extensions #2547

Merged

matzew mentioned this pull request Sep 27, 2022

MTChannelBroker: Enhance failed events with extra metadata knative/eventing#6541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Knative error extensions #2374

Knative error extensions #2374

egorlepa commented Jul 4, 2022 •

edited by matzew

Loading

linux-foundation-easycla bot commented Jul 4, 2022 •

edited

Loading

knative-prow bot commented Jul 4, 2022

knative-prow bot commented Jul 4, 2022

pierDipi commented Jul 4, 2022

codecov bot commented Jul 4, 2022 •

edited

Loading

matzew commented Jul 4, 2022

egorlepa commented Jul 4, 2022

matzew Jul 5, 2022

egorlepa Jul 5, 2022

matzew commented Jul 5, 2022

pierDipi commented Jul 6, 2022

matzew commented Jul 6, 2022

matzew Jul 6, 2022

matzew commented Jul 6, 2022

pierDipi Jul 6, 2022

pierDipi Jul 7, 2022

pierDipi Jul 7, 2022

matzew left a comment

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew left a comment

knative-prow bot commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

knative-prow-robot commented Aug 23, 2022

knative-prow-robot commented Aug 23, 2022

	return cetest.HasExtension("knativeerrordest", dlsAddress.Path)
	return cetest.HasExtension("knativeerrordest", dlsAddress.String())

	f.Setup("install source", eventshub.Install(
	f.Requirement("install source", eventshub.Install(

Knative error extensions #2374

Knative error extensions #2374

Conversation

egorlepa commented Jul 4, 2022 • edited by matzew Loading

linux-foundation-easycla bot commented Jul 4, 2022 • edited Loading

knative-prow bot commented Jul 4, 2022

knative-prow bot commented Jul 4, 2022

pierDipi commented Jul 4, 2022

codecov bot commented Jul 4, 2022 • edited Loading

Codecov Report

matzew commented Jul 4, 2022

egorlepa commented Jul 4, 2022

matzew Jul 5, 2022

Choose a reason for hiding this comment

egorlepa Jul 5, 2022

Choose a reason for hiding this comment

matzew commented Jul 5, 2022

pierDipi commented Jul 6, 2022

matzew commented Jul 6, 2022

matzew Jul 6, 2022

Choose a reason for hiding this comment

matzew commented Jul 6, 2022

pierDipi Jul 6, 2022

Choose a reason for hiding this comment

pierDipi Jul 7, 2022

Choose a reason for hiding this comment

pierDipi Jul 7, 2022

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Aug 23, 2022

matzew commented Aug 23, 2022

matzew commented Aug 23, 2022

knative-prow-robot commented Aug 23, 2022

knative-prow-robot commented Aug 23, 2022

egorlepa commented Jul 4, 2022 •

edited by matzew

Loading

linux-foundation-easycla bot commented Jul 4, 2022 •

edited

Loading

codecov bot commented Jul 4, 2022 •

edited

Loading