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 ability to deep-mind log to stderr #732

Merged
merged 5 commits into from
Sep 10, 2024
Merged

Add ability to deep-mind log to stderr #732

merged 5 commits into from
Sep 10, 2024

Conversation

heifner
Copy link
Member

@heifner heifner commented Sep 10, 2024

The theory is that #724 is caused by too much buffered output to the pipe. Add ability to write to stderr which is unbuffered.

Add an example logging.json that can be used to configure deep-mind to log to stderr.

Seems to resolve #724

@heifner heifner requested review from greg7mdp, spoonincode and arhag and removed request for greg7mdp September 10, 2024 01:15
@heifner heifner added the OCI Work exclusive to OCI team label Sep 10, 2024
libraries/libfc/src/log/dmlog_appender.cpp Show resolved Hide resolved
libraries/libfc/src/log/dmlog_appender.cpp Outdated Show resolved Hide resolved
Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

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

Adding the ability for deepmind to go to stderr seems useful but also seems like a new feature (i.e. not release branch material).

So far I've not seen any evidence that a backed up FILE buffer can cause this failure. From the testing I've done, when the buffer is full nodeos blocks as expected. Is there a way to reproduce this error just from buffering? The EBADF errno from the original report also would not immediately suggest a user-space buffering problem to me.

Since deepmind output is, I take it, always expected to be piped to some other process, maybe dmlog_appender should fflush after each output (or maybe more optimally at some other delineation point). That does seems like a reasonable thing we could add. But lack of that only means that worst case the consumer is a little "behind" the most recent deepmind output.

imo we need more understanding of the issue -- we can't just say deepmind only works properly on stderr instead of stdout.

@heifner heifner changed the base branch from release/1.0 to main September 10, 2024 10:51
@heifner heifner changed the title [1.0.1] Add ability to deep-mind log to stderr Add ability to deep-mind log to stderr Sep 10, 2024
@heifner
Copy link
Member Author

heifner commented Sep 10, 2024

@spoonincode re-targeted to main.

@ericpassmore ericpassmore added the enhancement New feature or request label Sep 10, 2024
@ericpassmore
Copy link
Contributor

Note:start
category: Logging
component: DeepMind
summary: Add the ability for deepmind to go to stderr, which is unbuffered.
Note:end

@heifner heifner merged commit c5fccff into main Sep 10, 2024
36 checks passed
@heifner heifner deleted the GH-724-dmlog branch September 10, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When starting firehose from a snapshot on kylin, nodeos terminates with DMLOG FPRINTF_FAILURE_TERMINATED
4 participants