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

Add logging for high keyframe interval, reduce log level for discovery loop #2709

Merged
merged 4 commits into from
Jan 5, 2023

Conversation

eliteprox
Copy link
Contributor

@eliteprox eliteprox commented Jan 5, 2023

What does this pull request do? Explain your changes. (required)

Specific updates (required)

  • Added logging to SegmenterTimeout error in media_server.go to suggest for the broadcaster to lower their keyframe interval. Confirmed this error often occurs when the keyframe interval is 8 seconds or higher.

    • RTMP Timeout: Ensure keyframe interval is less than 8 seconds
  • Reduced log level for orchestrator discovery loop during streaming to debug, this helps prevent excessive logging when broadcaster has only a few orchestrators. Example of issue: Broadcaster Discovery Loop Errors.txt

How did you test each of these updates (required)

  1. Broadcasted with and without -orchAddr using low deposit.
  2. Changed log level with -v flag. Confirmed discovery loop error logs will appear only if -v 5 or higher is used.
  3. Captured log behavior while streaming New Broadcasting Logs.txt

Does this pull request close any open issues?

None

Checklist:

@eliteprox eliteprox changed the title Add logging for high keyframes and Add logging for high keyframe interval, reduce log level for discovery loop Jan 5, 2023
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #2709 (9c91d1d) into master (cf95f00) will decrease coverage by 0.01929%.
The diff coverage is 10.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2709         +/-   ##
===================================================
- Coverage   56.35194%   56.33265%   -0.01929%     
===================================================
  Files             88          88                 
  Lines          19128       19131          +3     
===================================================
- Hits           10779       10777          -2     
- Misses          7761        7765          +4     
- Partials         588         589          +1     
Impacted Files Coverage Δ
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
server/mediaserver.go 67.35632% <0.00000%> (-0.20503%) ⬇️
discovery/discovery.go 91.66667% <100.00000%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82e40f9...9c91d1d. Read the comment docs.

Impacted Files Coverage Δ
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
server/mediaserver.go 67.35632% <0.00000%> (-0.20503%) ⬇️
discovery/discovery.go 91.66667% <100.00000%> (ø)

@eliteprox eliteprox marked this pull request as ready for review January 5, 2023 06:44
@Titan-Node
Copy link

Reduced log level for orchestrator discovery loop during streaming to debug, this helps prevent excessive logging when broadcaster has only a few orchestrators. Example of issue: Broadcaster Discovery Loop Errors.txt

By default a B will timeout all segments received equal or greater than 8 seconds.
We tried to find the variable for the RTMP timeout to allow OBS to send segments greater than 7 seconds (I think OBS defaults to 8 seconds) but couldn't find it. Unless you manually change OBS keyframe interval to less than 7 seconds it does not work. Hopefully this log will save people a lot of time.

Added logging to SegmenterTimeout error in media_server.go to suggest for the broadcaster to lower their keyframe interval. Confirmed this error often occurs when the keyframe interval is 8 seconds or higher.

Works great, really needed to clean up the logs while using the -pricePerBroadcaster set to 0 for free transcoding.

@leszko leszko self-requested a review January 5, 2023 07:54
Copy link
Contributor

@AlexKordic AlexKordic left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@cyberj0g cyberj0g left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomshutt thomshutt merged commit a5ccafa into livepeer:master Jan 5, 2023
@thomshutt thomshutt mentioned this pull request Feb 10, 2023
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.

None yet

5 participants