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

MTChannelBroker: Enhance failed events with extra metadata #6541

Closed
matzew opened this issue Sep 27, 2022 · 3 comments · Fixed by #6569
Closed

MTChannelBroker: Enhance failed events with extra metadata #6541

matzew opened this issue Sep 27, 2022 · 3 comments · Fixed by #6569
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/feature-request kind/good-first-issue Denotes an issue ready for a new contributor.

Comments

@matzew
Copy link
Member

matzew commented Sep 27, 2022

If a message is routed to the deadletter sink of the MTChannelBroker or its Triggers, there is no contextual information on why the message landed there.

In a few Knative components, we have some metadata for this:

Would be nice if the reference implementation of the MTChannelBroker would have this feature too

@matzew matzew added kind/feature-request kind/good-first-issue Denotes an issue ready for a new contributor. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Sep 27, 2022
@liuchangyan
Copy link
Contributor

/assign

@lionelvillard
Copy link
Member

We may want to go further by standardizing these attributes and adding them to the knative spec.

@gab-satchi @gabo1208 WDYT?

@liuchangyan
Copy link
Contributor

liuchangyan commented Oct 14, 2022

Hi, I am working on adding "knativeerrordest knativeerrorcode knativeerrordata" extensions on MTchannelBroker,
Most of the data in MTchannelBroker is processed by Channels:
In Control-Plane of MTchannelBroker, mt-broker-controller create subscription according to the information of the trigger, this subscription contains the address of subscriber(broker-filter) and the information of Broker, like this example:

apiVersion: messaging.knative.dev/v1
kind: Subscription
metadata:
  name: default-trigger1-3f6e7a8e-f7d1-4c8f-a25a-1970b6ab73e3
  namespace: default
spec:
  channel:
    apiVersion: messaging.knative.dev/v1beta1
    kind: NatssChannel
    name: default-kne-trigger
  reply:
    ref:
      apiVersion: eventing.knative.dev/v1
      kind: Broker
      name: default
      namespace: default
  subscriber:
    uri: http://broker-filter.knative-eventing.svc.cluster.local/triggers/default/trigger1/3f6e7a8e-f7d1-4c8f-a25a-1970b6ab73e3

So in the entire data flow, the path of event message is channel->broker-filter->target; and the path pf response is target->broker-filter->channel, and the extention "knativeerrordest" what we got is broker-filter's URI, but what we need is actual trigger's URI.

Based on this situation, there are two methods that we could get trigger's URI:

Method 1: In broker-filter, we could add a pair of key value in response body to store trigger's URI. After getting response Channel could unmarshal this URI when the destination is broker-filter.

Method 2: We could add processing logic about deadlettersink in broker-filter when sending error is not nil , do the same like Channel.

Which one is better? is there any other methods and what do you think? @pierDipi @matzew

knative-prow bot pushed a commit that referenced this issue Nov 16, 2022
Fixes #6541  
Signed-off-by: Teresaliu
[changyan.liu@intel.com](https://github.com/knative/eventing/pull/changyan.liu@intel.com)

<!-- Please include the 'why' behind your changes if no issue exists -->

## Proposed Changes

<!-- Please categorize your changes:
- 🎁 Add new feature
- 🐛 Fix bug
- 🧹 Update or clean up current behavior
- 🗑️ Remove feature or internal logic
-->

- Add Reconciler Test of Channel to test failer extensions metadata
- Add failed events extensions `knativeerrordest,
knativeerrordata,knativeerrorcode` to MTChannelBroker and corresponding
Reconciler Test to Broker

### Pre-review Checklist

<!-- If these boxes are not checked, you will be asked to complete these
requirements or explain why they do not apply to your PR. -->

- [ ] **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**

<!--
📄 If this change has user-visible impact, write a release
note in the block
below. Include the string "action required" if additional action is
required of
users switching to the new release, for example in case of a breaking
change.

Write as if you are speaking to users, not other Knative contributors.
If this
change has no user-visible impact, no release note is needed.
-->

```release-note
Add contextual information on why the message landed in deadletter sink of the MTChannel-based Broker or its Triggers.
```


**Docs**

<!--
📖 If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/feature-request kind/good-first-issue Denotes an issue ready for a new contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants