-
Notifications
You must be signed in to change notification settings - Fork 26
Feature/better logging #1413
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
base: develop
Are you sure you want to change the base?
Feature/better logging #1413
Conversation
| <uses-permission android:name="android.permission.INTERNET" /> | ||
| <uses-permission android:name="android.permission.CAMERA" /> | ||
| <uses-permission android:name="android.permission.USE_BIOMETRIC" /> | ||
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for the filesystem API
| remoteStrategyFactory: (otlpEndpoint: string, ingestionKey: string) => SigNozProvider = | ||
| (otlpEndpoint, ingestionKey) => new SigNozProvider(otlpEndpoint, ingestionKey), | ||
| delay: (ms: number) => Promise<void> = (ms) => new Promise(resolve => setTimeout(resolve, ms)), | ||
| syncMode: SyncMode = SyncMode.Manual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, syncing logs with the cloud is a manual process, so the user needs to confirm it using an Alert in the UI
| import { logger } from "../../utils/logger/Logger"; | ||
|
|
||
| export enum SyncMode { | ||
| Auto = "auto", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Auto mode can be very helpful for PoCs and other test cases
| await local.writeLogs(remainingLogs); | ||
| } | ||
|
|
||
| logger.debug("Logs sync attempt completed successfully.", undefined, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log wil be shown just in the console as consoleOnly param is true
| useEffect(() => { | ||
| if (initializationPhase === InitializationPhase.PHASE_TWO) { | ||
| if (authentication.loggedIn) { | ||
| if (logSyncService.syncMode === SyncMode.Auto) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are in Auto mode, logs will be auto sent to the cloud(PoC). By default the mode is always Manual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not just combine these changes with the useEffect directly above this for keriaNotification polling?
| setIsOpen(false); | ||
| }; | ||
|
|
||
| const handleShareLogs = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user shares the logs manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we share everything automatically but here we ask the user?
| export const useLogger = () => useContext(LoggerContext); | ||
|
|
||
| export const LoggerProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => ( | ||
| <LoggerContext.Provider value={logger}>{children}</LoggerContext.Provider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the context to use logs in the UI components
| const [openEditMembers, setOpenEditMembers] = useState(false); | ||
| const [loading, setLoading] = useState(false); | ||
| const history = useHistory(); | ||
| const logger = useLogger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we use the context
| console.debug(`LocalFileStrategy initialized. Log file name: ${this.logFile}`); | ||
|
|
||
| // Log the full path using console.info | ||
| Filesystem.getUri({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We console the path of the logs file in the device. We could avoid this in production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a problem imo
| ## iOS | ||
| APP_TEAM_ID=yourTeamID | ||
| # Logging Configuration | ||
| LOGGING_MODE=info # Possible values: off, debug, info, warn, error. If 'off', no logs will be processed. Otherwise, logs with a level lower than this will be ignored. Default is info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we store all these log variables in the configs? loca, remote, prod
f19edf3 to
e8ad3da
Compare
| private listener: PluginListenerHandle | undefined; | ||
| private localStrategyFactory: () => LocalFileStrategy; | ||
| private remoteStrategyFactory: (otlpEndpoint: string, ingestionKey: string) => SigNozProvider; | ||
| private delay: (ms: number) => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this as a variable? I would just inline new Promise(resolve => setTimeout(resolve, loggingConfig.retryDelayMs);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to be able to mock it in the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jest fake timers is usually the strategy then. If it causes issues we can move on though
| if (pending.length === 0) { | ||
| return; | ||
| } | ||
| for (let i = 0; i < pending.length; i += loggingConfig.batchSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loops too many times unless the batchSize is 1. e.g. if batch size is 10, and there are 10 items, it should loop once
| if (loggingConfig.signozOtlpEndpoint && loggingConfig.signozIngestionKey) { | ||
| const local = this.localStrategyFactory(); | ||
| const remote = this.remoteStrategyFactory(loggingConfig.signozOtlpEndpoint, loggingConfig.signozIngestionKey); | ||
| const pending: ParsedLogEntry[] = await local.readLogs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there are a massive amount of logs, will this variable not become huge?
|
|
||
| if (successfullySentLogIds.size > 0) { | ||
| const remainingLogs = pending.filter(log => !successfullySentLogIds.has(log.id)); | ||
| await local.writeLogs(remainingLogs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we delete them from local after syncing?
| } catch (error) { | ||
| logger.error(`Log sync attempt ${attempt} failed:`, { error, attempt }); | ||
| if (attempt < loggingConfig.maxSyncRetries) { | ||
| await this.delay(loggingConfig.retryDelayMs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this fails halfway through - it might re-send previous logs, right?
| }).then((result) => { | ||
| // eslint-disable-next-line no-console | ||
| console.info(`Full path to local log file: ${result.uri}`); | ||
| }).catch((e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. In this case we continue usage of the app but without logging. I wonder if we should just let it throw.
| // - Android: Use Android Studio > View > Tool Windows > Device File Explorer > /data/data/org.cardanofoundation.idw/files/local-logs.txt | ||
| } | ||
|
|
||
| async log(logEntry: ParsedLogEntry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File loggers will always write to rotate log files so they don't get too big, and eventually delete old ones where we will just have an ever growing log file. We need to consider how to handle this.
|
|
||
| // Log the full path using console.info | ||
| Filesystem.getUri({ | ||
| directory: Directory.Data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put a subdirectory here for logs maybe
|
|
||
| async log(logEntry: ParsedLogEntry) { | ||
| const entry = JSON.stringify(logEntry) + "\n"; | ||
| await Filesystem.appendFile({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how this handles the opened files? If it opens and closes for each log we need to think of another strategy - but if it maintains the file handle and automatically tidies up, we're OK - it just doesn't seem like that
| console.debug(`LocalFileStrategy initialized. Log file name: ${this.logFile}`); | ||
|
|
||
| // Log the full path using console.info | ||
| Filesystem.getUri({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a problem imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only adding these comments for now and I will take another look later once all other comments have been solved. It would be nice if you could add a little video demo.
| { | ||
|
|
||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This is not just an empty file, it's currently being overwritten with actual stuff when you build the whitelabel - Duke is currently working on a clean up.
| setIsOpen(false); | ||
| }; | ||
|
|
||
| const handleShareLogs = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we share everything automatically but here we ask the user?
Description
This PR introduces a new, modular logging system with configurable output strategies including console, local file, and remote (SigNoz). This enhancement significantly improves the application's observability and flexibility in managing logs.
Checklist before requesting a review
Issue ticket number and link
Testing & Validation
Design Review