search

RedHatInsights / event-schemas

WIP consoledot CloudEvents schemas
Apache License 2.0
4 stars 10 forks source link

Adding title and link to the event schema #11

Open josejulio opened 2 years ago

josejulio commented 2 years ago

We were discussing today about NOTIF-538 which requests a common way to display the event in the UI and link to them.

The proposal is to add the optional fields for title which describes the event in a human readable form such as: 10 recomendations found for system ABC. It will be up to the sender to define a title.

Other nice thing to have would be an URI to link back to the event or to see more information, this would be related to the event.

Right now the consumer of the event has to create a title by checking the event type and using the contents of the event to build a meaninful message.

//cc: @RedHatInsights/notifications-committers

theute commented 2 years ago

I'm wondering if we should limit in length the title... I don't have a good number in mind though, it will vary if this is displayed in Splunk vs Grafana vs Email subject vs Notif drawer... We can always truncate I guess...

pilhuhn commented 2 years ago

The idea is basically to put that in the envelope to make sure that every sub-schema provides it in the same way (if they decide to provide that at all)?

theute commented 2 years ago

Yes, so you can always display a summary of the event. They should be optional, but then some title should be generated from eventype/app name

theute commented 2 years ago

BTW a known issue is when you have multiple events per message... The title is set at the message level and would be the same for all events... For Policies we have all matching policies in a single message, they would all share the same title... "Policy matching for rhel8" for instance

theute commented 2 years ago

PS: I started a discussion here. I'm still hesitant because I'm not sure how useful it would be when there are multiple events in a message

josejulio commented 2 years ago

PS: I started a discussion here. I'm still hesitant because I'm not sure how useful it would be when there are multiple events in a message

We can also add the title/href for each event, json schema supports something like "your own object plus this other object" using the allOf syntax.

theute commented 2 years ago

Then I guess this would be much better !

PS: I started a discussion here. I'm still hesitant because I'm not sure how useful it would be when there are multiple events in a message

We can also add the title/href for each event, json schema supports something like "your own object plus this other object" using the allOf syntax.

josejulio commented 2 years ago

Updated the content - using the title and href_url as part of the event itself and the data. Changes might be easier to review with hide whitespace enabled.

I'm unsure about the name on the separate schema (human readable), would appreciate your thoughs.

theute commented 2 years ago

Congrats on finding a name, I don't have a better proposal.

I'm not a good json-schema reader so asking questions:

Changes sound fine to me and a lot more useful than my initial proposal!

PS: I know we'll have to think about i18n at some point but we'll evolve the schema I guess

(and thanks for the "hide whitespace" pro-tip, I learnt something today)

theute commented 2 years ago

@jeromemarc change request to the future schema, adding a text and URL to an event.

josejulio commented 2 years ago

I'm not a good json-schema reader so asking questions:

  • "human-readable" would not appear in the payload ? Only the 2 fields

That's correct, only the 2 fields. Wondering if we should have a top level property (like human_readable or something else) to make it harder to have collissions.

  • Are the 2 fields optional ? I think they should be optional as it may not makes sense for some events in particular the URL and we don't want people to start adding crap just to make the schema happy

They are optional, in json-schema everything is optional unless it's explicitely added to the required array.

  • Would "description" or "text" be a better field name than "title" ? Additional schemas will need to be careful about possible collisions anyway

I guess so, title seems so specific.

josejulio commented 2 years ago

Actually i missread the schema. In event level, the data is a single thing. It is the application that defines/allows to have multiple sub-events. This human_readable schema would need to be extended by all of them in the particular event.

But i guess we are back again to square one; the application (notifications or splunk) would need to know where to look for the sub-events.

kahowell commented 2 years ago

@josejulio I'm also thinking it probably makes sense to close #7 and incorporate anything necessary from that PR here. Thoughts?

theute commented 2 years ago

For daily aggregation, it's implemented on notifications side.

We introduced support to sent multiple events at once because of examples like this: 1 - a system payload is uploaded by a RHEL machine 2 - Advisor finds 5 recommendations for this payload When we store in splunk, user would prefer 5 distinct lines/events. When we address a human by email, he'll prefer 1 email with the 5 recommendations.

Adding support to send related events at once is a lot easier on our side.

The alternative is to stop supporting this, get individual events and do all the aggregation ourselves, this would make it a lot harder on our side I think. I know @gwenneg got familiar with Kafka streams and that my help...

I guess, for aggregated emails for a single payload we would need to 1 - collect events that share some same id 2 - wait for related events 3 - after a defined amount of time send the aggregated result (or an event claiming, it's the last one)

I don't know how that works when multiple pods are reading from Kafka and in case of failures during that process...

We'll also need to somewhat define what's the "id" that we want to look at, but we could have an additional field for an event-type that defines what field to use to aggregate events for emails, or part of the email template definition ?

jeromemarc commented 2 years ago
  • Would "description" or "text" be a better field name than "title" ? Additional schemas will need to be careful about possible collisions anyway

I guess so, title seems so specific.

I would probably change title to description as long as there is no conflict with other fields.

josejulio commented 2 years ago

@josejulio revisiting this, I get the impression that the goal of this is to add a re-usable schema for emitting aggregations. Is the expectation that the apps themselves will do the aggregations?

One issue we had with splunk is that they were consuming notifications from policies, drift and advisor (AFAIK), but they needed some sort of title/description and a link for their UI.

They could pick the app and event names (policy - policy triggered) or they could dig down on the data of each particular notification to build one (Policy "Foobar" triggered for system "System-003").

The aim here is to add a schema to allow (but not enforce) some kind of UI-ready metadata without having to know much about the contents for the consumers of the events. In the end, the consumer of the event could check somehow if the event conforms to that schema and use the fields.

@theute commented about the problem we have with allowing (and not allowing) multiple events within a single notification.

Not really a problem about the aggregation.

@josejulio I'm also thinking it probably makes sense to close #7 and incorporate anything necessary from that PR here. Thoughts?

The purpose of this PR is different from #7. But I can happily take over #7 (or create a new PR) if you want.

kahowell commented 2 years ago

@josejulio given you approved #7, I went ahead and merged it. Please feel free to ~rework this PR to make additional changes, or close this one if you like~ make further changes to notifications schema as you see fit. (edit: re-read that this PR has a different purpose from #7).

josejulio commented 2 years ago

Updated the code, sort of follows what #4 requires - but instead of a human readable representation for the event-type it aims to help provide a human readable representation for the event itself.

Suppose we have the event type cost-management-report-ready, the human readable schema could be used to include:

"data": {
  "title": "Cost management monthly report is ready",
  "href_url": "https://link-to-cost-management-monthly-reports/123b/foobar.pdf"
}

If we have a advisor-recommendation, the human readable schema could extend the advisor schema to include it. e.g.:

"data": {
  "advisor_recommendations": [ "/* all recommendations */" ]
  "title": "15 recommendations for system MySystem",
  "href_url": "https://inventory/MySystem"
}

It could go further than that and include the human_readable as part of each recommendation - it's up to the application how to use this schema.

"data": {
  "advisor_recommendations": [
    {
      "rule_id": "abc-001",
      "...",
      "title": "Recomendation abc-001 do not share your password on system MySystem",
      "href_url": "https://advisor/recommendations/abc-001"
    }
  ]
}

Current code extends the objects with title and href_url, but could live under a property (e.g. human_readable) - that might help to keep more order.

tagging @beav as the reporter of #4