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

Rolling log file writer in new io module #406

Merged
merged 18 commits into from
Nov 20, 2024

Conversation

rocketraman
Copy link
Contributor

A simple implementation to write request logs to files.

Implemented as a rolling system to avoid any one log file becoming too large, and setting an upper bound on device storage. The roll parameters are configurable.

This is useful for debugging user devices in production because apps can contain a feature to send these saved logs to a server for analysis.

Resolves #405 .

@xxfast
Copy link

xxfast commented Nov 4, 2024

Great work 👍 Wondering how we can prioritise this for the next release 🤔

@rocketraman
Copy link
Contributor Author

Great work 👍 Wondering how we can prioritise this for the next release 🤔

Thanks! You might want to post your interest on the parent issue, it might get more visibility there.

@KevinSchildhorn
Copy link
Contributor

For my changes:

  • Called apiDump
  • Changed libs.kotlinx.io from implementation to api as you cannot add the library without them ( was going to do the same for libs.kotlinx.datetime but I want to prevent version mismatches as some people already use datetime)
  • Re-ordered variables in RollingFileLogWriter so that the non-optional is first
  • Added new LogWriter to sample app, and adding mobileMain sourceset to handle new LogWriter
  • renaming logPath to logFilePath and logPrefix to logFileName

@KevinSchildhorn
Copy link
Contributor

KevinSchildhorn commented Nov 19, 2024

Very nice work, however we're running into cinterop issues when adding this change to the sample app, which causes a build error when testing. We want to ensure it works and builds properly before merging it in, so I'm holding off until we can fix this issue. The issues arise when building the sample app. At first it was an issue with the cinterop:

e: KLIB resolver: Could not find "co.touchlab:kermit-core-cinterop-os_log" in [/Users/kevinschildhorn/Documents/GitHub/Kermit/samples/sample, /Users/kevinschildhorn/.konan/klib, /Users/kevinschildhorn/.konan/kotlin-native-prebuilt-macos-aarch64-2.0.21/klib/common, /Users/kevinschildhorn/.konan/kotlin-native-prebuilt-macos-aarch64-2.0.21/klib/platform/ios_arm64]

However recently the error is:

> Following dependencies exported in the podDebugFramework binary are not specified as API-dependencies of a corresponding source set:
  
  Files: [/Users/kevinschildhorn/.gradle/caches/modules-2/files-2.1/co.touchlab/kermit-simple-iosarm64/2.0.4/704ee2e118d3850b5d4eb1b3148a2f306d0e9917/kermit-simple.klib]
  Files: [/Users/kevinschildhorn/.gradle/caches/modules-2/files-2.1/co.touchlab/kermit-simple-iosarm64/2.0.4/704ee2e118d3850b5d4eb1b3148a2f306d0e9917/kermit-simple.klib]
  
  Please add them in the API-dependencies and rerun the build.

@KevinSchildhorn KevinSchildhorn marked this pull request as draft November 19, 2024 18:46
@rocketraman
Copy link
Contributor Author

Very nice work, however we're running into cinterop issues when adding this change to the sample app, which causes a build error when testing. We want to ensure it works and builds properly before merging it in, so I'm holding off until we can fix this issue.

How can I reproduce this? I've done build on the sample project without any errors locally.

@KevinSchildhorn KevinSchildhorn marked this pull request as ready for review November 20, 2024 14:40
@KevinSchildhorn
Copy link
Contributor

Nevermind the issue was resolved, an issue with default heriarchy.

@KevinSchildhorn KevinSchildhorn merged commit c9af0b7 into touchlab:main Nov 20, 2024
2 checks passed
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.

Feature request: log to files
4 participants