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 support for file based logging in ios and macos #1713

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

jmwample
Copy link
Contributor

@jmwample jmwample commented Dec 3, 2024

As logs are not available to iOS or MacOS apps through the console, this PR makes it possible for logs to be written to file.

On iOS if a path is provided in the "IOS_LOG_FILEPATH" environment variable this function will attempt to open that file and use it as the logging sink. On MacOS logs are written to the static "/var/log/nym-vpnd/daemon.log".

If we are unable to open the log file for either iOS or MacOS we default to writing to the default (console) output.


Edit: the oslog crate that we are using for logging in iOS/MacOS does not support switching the sink/output of the logging as OSLog is a specific apple logging subsystem. We can possibly just use something like env_logger or pretty_env_logger to write to file, but it seems that this may not be necessary as iOS apps may be able to read from their own logs to the OSLog subsystem (i.e. based on net.nymtech.vpn.agent ).


This change is Reviewable

@jmwample jmwample changed the title Add support for file based logging in ios and macos [WIP] Add support for file based logging in ios and macos Dec 3, 2024
@jmwample jmwample changed the title [WIP] Add support for file based logging in ios and macos Add support for file based logging in ios and macos Dec 4, 2024
@rokas-ambrazevicius
Copy link
Contributor

    func getLogEntries() throws -> [OSLogEntryLog] {
        let rustSubsystem = "net.nymtech.vpn.agent" // The same subsystem used in Rust

        // Open the log store.
        let logStore = try OSLogStore(scope: .currentProcessIdentifier)

        // Get all the logs from the last hour.
        let oneHourAgo = logStore.position(date: Date().addingTimeInterval(-360000))

        // Fetch log objects.
        let allEntries = try logStore.getEntries(at: oneHourAgo)

        // Filter the log to be relevant for the Rust process.
        return allEntries
            .compactMap { $0 as? OSLogEntryLog }
//            .filter { $0.subsystem == rustSubsystem }
    }

Tried that a couple of times, but this never returns logs that we are interested in.

We are missing a uniffi call to pass in the log path?

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jmwample and @rokas-ambrazevicius)


nym-vpn-core/crates/nym-vpn-lib/src/platform/swift.rs line 8 at r1 (raw file):

use log::LevelFilter;
use oslog::OsLogger;
use pretty_env_logger::env_logger::fmt::Target;

Given that we already use tracing a lot, I think it would make sense to stick to it especially because it implements extensible plugin system capable of writing to file, oslog, eventlog, and many other target destinations.

nym-vpnd/src/logging.rs has example of how to set up logging to file and I think we could also leverage tracing-oslog instead of oslog::OsLogger for better integration with tracing.


nym-vpn-core/crates/nym-vpn-lib/src/platform/swift.rs line 12 at r1 (raw file):

/// Path used for MacOS logs
#[cfg(target_os = "macos")]
const MACOS_LOG_FILEPATH: &str = "/var/log/nym-vpnd/daemon.log";

It would be better to accept PathBuf as argument toinit_logs() since this is not a daemon, but a static library that's linked into each process on iOS. Each process will provide individual path to log file to which the sandboxed executable has access to.


nym-vpn-core/crates/nym-vpn-lib/src/platform/mod.rs line 139 at r1 (raw file):

}

pub fn init_logger() {

Missing PathBuf to log file on iOS/macOS.

@jmwample
Copy link
Contributor Author

jmwample commented Dec 4, 2024

My current concerns are that this may be between a rock and a hard place. If we use NonBlockingWriter then we have to store the WorkerGuard otherwise we may miss some logs around crashes / panics. If we instead use RollingFileAppender we may end up suffering because of slow blocking IO writes on some devices.

Also I don't know if / when it is used, but the uniffi exported function nym_vpn_lib::platform::mod::configureLib could cause a panic (the .init() functions of the subscribers are not meant to be called more than once) or remove logging to file and go back to writing to stdout because it doesn't get the path argument.

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 8 files at r2, 1 of 2 files at r3.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @jmwample and @rokas-ambrazevicius)


a discussion (no related file):
I am finally back at reviewing this.

The scope of this work was much smaller in the beginning. I think we should focus on solving two specific problems without trying to grasp 5 platforms or unify logging initialization because this seems to be difficult.

  • Make it possible to log to file on iOS
  • Log to file on macOS

The steps that I would take to achieve both:

  • Make it possible to pass a path to file on iOS (nym-vpn-lib) via call to uniffi, i.e modify fn initLogger() to accept Option<PathBuf> and if set, use the given path for writing log in it (configure tracing). If you need to store a WorkerGuard, put it in the lazy_static! section in platform/mod.rs.
  • On macOS, when _as_service == true, instead of calling into nym_vpn_lib::swift::init_logs(..) add logging to oslog with the new tracing plugin.

We can look into rolling strategy in a separate PR as we need logs like yesterday. Also we don't really care about double initialization as this should never happen.

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @jmwample and @rokas-ambrazevicius)

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 8 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @jmwample and @rokas-ambrazevicius)

Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @pronebird and @rokas-ambrazevicius)


a discussion (no related file):

Previously, pronebird (Andrej Mihajlov) wrote…

I am finally back at reviewing this.

The scope of this work was much smaller in the beginning. I think we should focus on solving two specific problems without trying to grasp 5 platforms or unify logging initialization because this seems to be difficult.

  • Make it possible to log to file on iOS
  • Log to file on macOS

The steps that I would take to achieve both:

  • Make it possible to pass a path to file on iOS (nym-vpn-lib) via call to uniffi, i.e modify fn initLogger() to accept Option<PathBuf> and if set, use the given path for writing log in it (configure tracing). If you need to store a WorkerGuard, put it in the lazy_static! section in platform/mod.rs.
  • On macOS, when _as_service == true, instead of calling into nym_vpn_lib::swift::init_logs(..) add logging to oslog with the new tracing plugin.

We can look into rolling strategy in a separate PR as we need logs like yesterday. Also we don't really care about double initialization as this should never happen.

Some good points here. Great to see work to sort out a unified logging approach, though for now a quick solution as an interim step would be good

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