feat(live-status-gw): expose pieces and split adlibs to a separate topic

ianshade commented 5 months ago

About the Contributor

This PR is being opened on behalf of TV 2 Norge.

Type of Contribution

This is a:

Feature

Current Behavior

The activePlaylist topic in the LSG contains a (usually long) array of adLibs that are changing relatively rarely compared to other information in that topic like the current/next parts.
Information about pieces is not available through the LSG.
Events from the activePlaylist topic may be sent more often than necessary, sometimes featuring incomplete data when the collection data hasn't settled yet e.g. during a Take.

New Behavior

Adlibs for the active playlist are now available in the adLibs topic.
Currently playing Pieces from the On Air Part are available in the activePieces topic:
- Updated in real time (driven by the properties set by Core based on the pieceInstance callbacks from the Playout Gateway).
Throttling and incomplete data prevention are implemented.
publicData property on the AdLibs, Pieces, Parts and Segments, and the Playlist is exposed

Testing Instructions

Other Information

This is a breaking change in the Live Status Gateway API and wasn't preceded by an RFC so I'll appreciate any feedback on whether this is a step in the right direction for the users of the LSG

Status

[x] PR is ready to be reviewed.
[x] The functionality has been tested by the author.
[x] Relevant unit tests has been added / updated.
[x] Relevant documentation (code comments, system documentation) has been added / updated.

jstarpl commented 5 months ago

Hello!

Thank you for contributing to the Sofie Project!

If you haven’t already, please give our contribution guidelines a read. We will reach out to you to schedule an initial discussion regarding this (more details to follow).

jstarpl commented 5 months ago

Hi!

The NRK team has no objections to this change per se - moving the AdLibs onto a separate topic seems a sensible decision. One could argue that having the Active Pieces exposed on the activePlaylist topic might also be too much information, and maybe could instead be branched off to their own activePlaylistPieces topic, allowing the consumer a more fine-grained control over the data being pushed. That being said, NRK is not using this feature set at the moment, so we don't feel very strongly about what will make sense.

The more important case is that this is a breaking change, targeting 1.51 in a feature that lands in 1.50, potentially breaking any software that might integrate against Live Status Gateway, if they rely on the adLibs property being present. Because of that, we would like to invite others using Live Status Gateway - especially the original contributor - to bring forward their positions on this change in the comments section of this PR.

We'll keep this PR open until 2024/02/06.

jstarpl commented 5 months ago

Actually, I do have one proper comment: since this would be a breaking change, I think this version number should be major-bumped as well:

https://github.com/nrkno/sofie-core/pull/1131/files#diff-29807ab8c853f30bb20871ec7181b4002c60a75f9fc186d0c9690fa54b495a42R5

scriptorian commented 5 months ago

This all looks like good progress. There are many other changes before this set that my customer is not up-to-date with - I have reviewed these as well and have no concerns. I would prefer to see the active pieces in their own topic as well - it seems more logical to me - but I imagine there are few cases where there are a large number of pieces per part so this is not a real concern. Yes please to a major version bump. We could consider adding a version string to the connect path if we need to support multiple versions in the future.

jstarpl commented 5 months ago

@ianshade How would you feel about splitting off the active pieces to their own topic. Would that be okay with you?

ianshade commented 5 months ago

@jstarpl @scriptorian I like this idea. I'm all for splitting the active pieces to their own topic.

PeterC89 commented 5 months ago

This looks great, we're about to start working on some bits that will integrate with live status gateway and this brings some much needed functionality on our end! I can't quite tell if this will include the newly implemented publicDataproperty on the parts / pieces? Is that part of this PR or a future concern?

ianshade commented 5 months ago

@PeterC89, publicData is not included, but if there is no objection, I can add it as part of this PR

jstarpl commented 5 months ago

@PeterC89, publicData is not included, but if there is no objection, I can add it as part of this PR

I'm gonna go out on a limb, and say that I don't expect anyone from the NRK team to object.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 149 lines in your changes are missing coverage. Please review.

Comparison is base (3ad0c73) 57.74% compared to head (12779cb) 58.02%. Report is 59 commits behind head on release51.

Files	Patch %	Lines
meteor/server/migration/1_50_0.ts	28.91%	59 Missing :warning:
...ations/packageManager/expectedPackages/generate.ts	0.00%	29 Missing :warning:
...job-worker/src/blueprints/context/OnTakeContext.ts	82.66%	26 Missing :warning:
...text/services/PartAndPieceInstanceActionService.ts	97.78%	12 Missing :warning:
meteor/server/publications/rundown.ts	0.00%	7 Missing :warning:
...ns/pieceContentStatusUI/rundown/regenerateItems.ts	0.00%	5 Missing :warning:
...s/server-core-integration/src/lib/subscriptions.ts	16.66%	5 Missing :warning:
...orker/src/blueprints/context/OnSetAsNextContext.ts	96.61%	4 Missing :warning:
.../job-worker/src/blueprints/context/adlibActions.ts	95.34%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## release51 #1131 +/- ## ============================================= + Coverage 57.74% 58.02% +0.28% ============================================= Files 512 517 +5 Lines 82571 83403 +832 Branches 4307 4353 +46 ============================================= + Hits 47679 48398 +719 - Misses 34841 34951 +110 - Partials 51 54 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ianshade commented 5 months ago

I extracted active pieces to a separate topic, exposed publicData where applicable, and bumped the version. Edit: Looks like I forgot to add publicData to the API definitions

jstarpl commented 5 months ago

@ianshade I've updated the "New behavior" section of the PR description, please make sure I didn't mischaracterize something.

ianshade commented 4 months ago

I've updated the "New behavior" section of the PR description, please make sure I didn't mischaracterize something

@jstarpl, thanks, I added that the Playlist is among the documents that have their publicData exposed.

The asyncapi docs now reflect the publicData addition.

nrkno / sofie-core