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

[Player] Use event producer #92

Open
wants to merge 29 commits into
base: add-event-sender
Choose a base branch
from

Conversation

enjikaka
Copy link
Member

  • Use Event Producer for streaming metrics, play log and playback events
  • Remove setClientPlatform and setAppVersion as that info is gathered from Event Producer config. (And that is only needed for the "old nested header" which should be removed...?)
  • Remove old event beacon worker

@enjikaka enjikaka self-assigned this Mar 22, 2024
@enjikaka enjikaka requested a review from a team as a code owner March 22, 2024 14:42
@enjikaka enjikaka changed the base branch from main to add-event-sender March 22, 2024 14:42
@enjikaka enjikaka marked this pull request as draft March 25, 2024 09:23
@@ -1,2 +1,38 @@
export "$(grep -vE "^(#.*|\s*)$" .env)"
TEST_USER=$TEST_USER npm run wtr $1
# TEST_USER=$TEST_USER npm run wtr $1
# Event Producer seems to prevent WTR from running all tests in one go, so we need to run them one by one
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web Test Runner wouldn't run normally but running each test individually worked, so I succumbed to this hack.

I believe Event Producer is the culprit, doesn't seem it's made for testing in mind. After my first attempts of running the tests I got its IDB filled with 300 000 items and Chrome hang on memory errors. Also got "Crash: Failed to execute 'postMessage' on 'MessagePort': Data cannot be cloned, out of memory.", I suppose EP tried sending those 300k events over the MessagePort too... Tried adding proper cleanups of the IDBs after each test but it still refused to run. :(

@enjikaka enjikaka marked this pull request as ready for review April 8, 2024 08:09
@andymartinwork
Copy link
Contributor

You might want to hold off on this until we've confirmed the Event Producer is working properly

@andymartinwork
Copy link
Contributor

OK, seems Event Producer is working for CDF events now

@osmestad
Copy link
Contributor

osmestad commented May 2, 2024

A bit worried as there is no feature flag to switch back, do we dare to do this with needing a release to be able to roll back if it breaks?

@enjikaka
Copy link
Member Author

enjikaka commented May 3, 2024

A bit worried as there is no feature flag to switch back, do we dare to do this with needing a release to be able to roll back if it breaks?

I'd say it's way easier to roll back a version for our own client than to introduce feature flags in this repo and keep the code side by side. That would grow the bug surface for the release and complicate the API for third parties too...

*/
export async function commit(baseCommitData: BaseCommitData) {
/*
This is a workaround to keep the old nested header structure,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the deal with this? I think Haffy might have added a workaround to the SDK if it's the same problem I'm thinking of?

Copy link
Member Author

@enjikaka enjikaka May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back-end apparently doesn't use the "enriched header" from event producer yet so I still need these props as well. But if there is a built in work around for that now that'd be nice...

eventSenderStore.eventSender.sendEvent(event);

if (streamingSessionId) {
await db.delete({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this only mean there can be one event type at a time in the db?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be many! They are scoped by name and streamingSessionId. Here's the wrapper: https://github.com/tidal-music/tidal-sdk-web/blob/main/packages/player/src/internal/helpers/event-session.ts

@andymartinwork
Copy link
Contributor

I've had a look through the code here. Give me a bell before you merge this and we can test the events end to end

@with-joy
Copy link

streaming metrics cannot be migrated to EP yet. The backend infra does not support enhancing the event payload with client ip which @tidal-gaute and team need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants