Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"Airplus",
"airshipconfig",
"airside",
"alrt",
"Amal",
"Amal's",
"americanexpressfdx",
Expand Down
3 changes: 3 additions & 0 deletions src/libs/Log.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {shouldAttachLog} from './Console';
import getPlatform from './getPlatform';
import {post} from './Network';
import requireParameters from './requireParameters';
import forwardLogsToSentry from './telemetry/forwardLogsToSentry';

let timeout: NodeJS.Timeout;
let shouldCollectLogs = false;
Expand Down Expand Up @@ -59,6 +60,8 @@ function serverLoggingCallback(logger: Logger, params: ServerLoggingCallbackOpti
if (requestParams.parameters) {
requestParams.parameters = JSON.stringify(requestParams.parameters);
}
// Mirror backend log payload into Telemetry logger for better context
forwardLogsToSentry(requestParams.logPacket);
clearTimeout(timeout);
timeout = setTimeout(() => logger.info('Flushing logs older than 10 minutes', true, {}, true), 10 * 60 * 1000);
return LogCommand(requestParams);
Expand Down
69 changes: 69 additions & 0 deletions src/libs/telemetry/forwardLogsToSentry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import * as Sentry from '@sentry/react-native';

type SentryLogLevel = 'debug' | 'info' | 'warn' | 'error';

/**
* Method deciding whether a log packet should be forwarded to Sentry.
*
* ATTENTION!
* Currently, this always returns false because we want to deliberately decide what is being forwarded.
* There is no redaction / filtering of sensitive data implemented yet. When you implement any log forwarding logic, make sure that you do not leak any sensitive data.
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function shouldForwardLog(log: {message?: string; parameters?: Record<string, unknown> | undefined}) {
return false;
}

function mapLogMessageToSentryLevel(message: string): SentryLogLevel {
if (message.startsWith('[alrt]')) {
return 'error';
}
if (message.startsWith('[warn]') || message.startsWith('[hmmm]')) {
return 'warn';
}
if (message.startsWith('[info]')) {
return 'info';
}
return 'debug';
}
Comment on lines +17 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're connecting Sentry logging to server log event so the input is string here. To decide what severity of logs should be logged I parse the log entry. Maybe you see better place to put forwarding so we can have all info without parsing the string?


function forwardLogsToSentry(logPacket: string | undefined) {
if (!logPacket) {
return;
}

let parsedPacket: Array<{message?: string; parameters?: Record<string, unknown> | undefined}> | undefined;
try {
parsedPacket = JSON.parse(logPacket) as typeof parsedPacket;
} catch {
Sentry.logger.warn('Failed to parse log packet for Sentry forwarding', {logPacket});
return;
}

if (!Array.isArray(parsedPacket)) {
return;
}

for (const logLine of parsedPacket) {
if (!logLine || typeof logLine.message !== 'string') {
continue;
}
if (!shouldForwardLog(logLine)) {
continue;
}

const level = mapLogMessageToSentryLevel(logLine.message);
const logMethod = Sentry.logger[level];
if (!logMethod) {
continue;
}

if (logLine.parameters) {
logMethod(logLine.message, logLine.parameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

how are we handling any redacting in this case? in web we do have reduction logic before the logs are processed, how can we add that to Sentry @rlinoz @sosek108

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good point, although I was thinking this would follow something along the lines of what we have in Auth, where we just redact parameters that are not whitelisted.

So we would do something related to that in a follow up PR where we fill the shouldForwardLog function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where we just redact parameters that are not whitelisted.

I think this is good idea.

in web we do have reduction logic before the logs are processed

only web? what's about mobile? Maybe we could move this log forwarding after data is redacted via expensify's mechanisms?

Copy link
Contributor

Choose a reason for hiding this comment

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

only web? what's about mobile? Maybe we could move this log forwarding after data is redacted via expensify's mechanisms?

Mobile sends to Web-E and then Web-E actually logs it, so I think this would be cumbersome, it would go something like

  1. Call Log command
  2. Wait for a response
  3. Send the log to Sentry

This would probably make logs not follow the correct timeline I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so we'll do redaction in follow up PR.

If you're ok with: we just redact parameters that are not whitelisted. I will do so

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm but if we add redaction later there will be a version of the app with no redaction of the logs, not sure if I am following. We should make sure we redact them from beginning as we dont want to be dealing with removing sensitive data from Sentry so early 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial line of thought was that:

If we don't send anything there is nothing to redact and redaction should be added alongside with logs that we add to sending.

But as I understand this thinking is shady we can copy the redaction from web (mentioned here) and then push this PR to merge.

} else {
logMethod(logLine.message);
}
}
}

export default forwardLogsToSentry;
1 change: 1 addition & 0 deletions src/setup/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export default function (): void {
environment: CONFIG.ENVIRONMENT,
release: `${pkg.name}@${pkg.version}`,
beforeSendTransaction: processBeforeSendTransactions,
enableLogs: true,
});

startSpan(CONST.TELEMETRY.SPAN_APP_STARTUP, {
Expand Down
Loading