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

KERNEL: Refine logger and error #913

Closed
winlinvip opened this issue Jun 9, 2017 · 3 comments
Closed

KERNEL: Refine logger and error #913

winlinvip opened this issue Jun 9, 2017 · 3 comments
Assignees
Labels
EnglishNative This issue is conveyed exclusively in English. Enhancement Improvement or enhancement.
Milestone

Comments

@winlinvip
Copy link
Member

The logger should only log the "BIG" events, which are not error. In addition, logger should support context. The errors should, not only the error code, but indicates the context such as the stack and description. For detail information about errors, please read https://gocn.io/article/348.

For SRS, it confuses the error and logger, that is SRS output error in logger and in each function calls. There is a example, when error cames from SrsSource::on_video_imp, each function will print a error:

int SrsSource::on_video_imp(SrsSharedPtrMessage* msg) {
    if ((ret = hub->on_video(msg, is_sequence_header)) != ERROR_SUCCESS) {
        srs_error("origin hub error, ret=%d", ret);
        return ret;
    }
}

int SrsOriginHub::on_video(SrsSharedPtrMessage* shared_video, bool is_sequence_header) {
    if ((ret = format->on_video(msg)) != ERROR_SUCCESS) {
        srs_error("Codec parse video failed, ret=%d", ret);
        return ret;
    }
}

int SrsFormat::on_video(int64_t timestamp, char* data, int size) {
    return video_avc_demux(buffer, timestamp);
}

int SrsFormat::video_avc_demux(SrsBuffer* stream, int64_t timestamp) {
    if (!stream->require(4)) {
        ret = ERROR_HLS_DECODE_ERROR;
        srs_error("avc decode avc_packet_type failed. ret=%d", ret);
        return ret;
    }
}

Let's have a look what happends when there is a decode error:

avc decode avc_packet_type failed. ret=3001
Codec parse video failed, ret=3001
origin hub error, ret=3001

Is it useful? Yes, it's useful than just an error code like Failed, code is 3001, but it's not enough:

  1. There is no stack info.
  2. The logs are not simply enough.

I'll be better if the error is like this:

Error processing video, code=3001 : origin hub : codec parser : avc decoder
[100] video_avc_demux() at [srs_kernel_codec.cpp:676]
[100] on_video() at [srs_app_source.cpp:1076]
[101] on_video_imp() at [srs_app_source:2357]

For logger, it should log some important events, the error should be one event.

@winlinvip winlinvip added the Enhancement Improvement or enhancement. label Jun 9, 2017
@winlinvip winlinvip added this to the srs 3.0 release milestone Jun 9, 2017
@winlinvip
Copy link
Member Author

winlinvip commented Jun 9, 2017

We use variable ret:int as simple error code, while using err:srs_error_t for complex error. In addition, use macro ERROR_SUCCESS:int and srs_success:srs_error_t.

For some error in underlayer, for example, for daemon to fork processes, we can just use error without logger:

    if(pid < 0) {
        return srs_error_new(-1, "fork failed");
    }

Replace the old logger+error:

    if(pid < 0) {
        srs_error("create process error. ret=-1"); //ret=0
        return -1;
    }

That's the point. After refined, the error is a single event to log:

[2017-06-09 11:47:44.954][Error][65476][0][0] Failed, code=1005 : server initialize : http api initialize
thread #0: run() [src/main/srs_main_server.cpp:375][errno=0]
thread #0: initialize() [src/app/srs_app_server.cpp:583][errno=0]

For each call function, use srs_error_wrap to attach the stack:

    if ((err = svr->initialize_st()) != srs_success) {
        return srs_error_wrap(err, "initialize st");
    }

That's right~

winlinvip added a commit that referenced this issue Jun 9, 2017
winlinvip added a commit that referenced this issue Jun 9, 2017
winlinvip added a commit that referenced this issue Jun 10, 2017
winlinvip added a commit that referenced this issue Jun 10, 2017
winlinvip added a commit that referenced this issue Jun 11, 2017
@winlinvip
Copy link
Member Author

Here we confused the log and error:

            if ((ret = rtsp->send_message(res)) != ERROR_SUCCESS) {
                if (!srs_is_client_gracefully_close(ret)) {
                    srs_error("rtsp: send SETUP response failed. ret=%d", ret);
                }
                return ret;
            }

Instead, we should return a error:

            if ((ret = rtsp->send_message(res)) != ERROR_SUCCESS) {
                return srs_error_new(ret, "send message");
            }

Finally at the top of call, such as the thread cycle or main cycle, we can decide whether to print the error or not.

winlinvip added a commit that referenced this issue Jul 29, 2017
winlinvip added a commit that referenced this issue Sep 22, 2017
winlinvip added a commit that referenced this issue Sep 24, 2017
winlinvip added a commit that referenced this issue Jan 1, 2018
winlinvip added a commit that referenced this issue Jan 1, 2018
winlinvip added a commit that referenced this issue Jan 1, 2018
winlinvip added a commit that referenced this issue Jan 1, 2018
@winlinvip
Copy link
Member Author

Fixed.

@winlinvip winlinvip self-assigned this Sep 15, 2021
@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English. Enhancement Improvement or enhancement.
Projects
None yet
Development

No branches or pull requests

1 participant