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

fix: action and webhook logs improvements #2634

Merged
merged 2 commits into from
Aug 26, 2024
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
6 changes: 0 additions & 6 deletions packages/jobs/lib/execution/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,6 @@ export async function startAction(task: TaskAction): Promise<Result<void>> {
}
}
export async function handleActionSuccess({ nangoProps }: { nangoProps: NangoProps }): Promise<void> {
const logCtx = await logContextGetter.get({ id: String(nangoProps.activityLogId) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be a relieve for the infra I think 👌🏻

const content = `${nangoProps.syncConfig.sync_name} action was run successfully and results are being sent synchronously.`;

await logCtx.info(content);

const connection: NangoConnection = {
id: nangoProps.nangoConnectionId!,
connection_id: nangoProps.connectionId,
Expand Down Expand Up @@ -219,5 +214,4 @@ async function onFailure({
}
});
}
await logCtx.error(error.message, { error });
}
1 change: 0 additions & 1 deletion packages/jobs/lib/execution/postConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ export async function handlePostConnectionSuccess({ nangoProps }: { nangoProps:
createdAt: Date.now()
});
const logCtx = await logContextGetter.get({ id: String(nangoProps.activityLogId) });
await logCtx.info(content);
await logCtx.success();
}

Expand Down
8 changes: 0 additions & 8 deletions packages/jobs/lib/execution/webhook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ export async function startWebhook(task: TaskWebhook): Promise<Result<void>> {
syncName: task.parentSyncName,
syncJobId: syncJob?.id,
providerConfigKey: task.connection.provider_config_key,
activityLogId: task.activityLogId,
runTime: 0,
error,
environment: { id: task.connection.environment_id, name: environment?.name || 'unknown' },
Expand Down Expand Up @@ -151,8 +150,6 @@ export async function handleWebhookSuccess({ nangoProps }: { nangoProps: NangoPr
runTimeInSeconds: (new Date().getTime() - nangoProps.startedAt.getTime()) / 1000,
createdAt: Date.now()
});
const logCtx = await logContextGetter.get({ id: String(nangoProps.activityLogId) });
await logCtx.info(content);

await updateSyncJobStatus(nangoProps.syncJobId!, SyncStatus.SUCCESS);
}
Expand All @@ -169,7 +166,6 @@ export async function handleWebhookError({ nangoProps, error }: { nangoProps: Na
syncName: nangoProps.syncConfig.sync_name,
syncJobId: nangoProps.syncJobId!,
providerConfigKey: nangoProps.providerConfigKey,
activityLogId: nangoProps.activityLogId!,
runTime: (new Date().getTime() - nangoProps.startedAt.getTime()) / 1000,
error,
environment: { id: nangoProps.environmentId, name: nangoProps.environmentName || 'unknown' },
Expand All @@ -185,7 +181,6 @@ async function onFailure({
syncName,
syncJobId,
providerConfigKey,
activityLogId,
runTime,
error
}: {
Expand All @@ -196,7 +191,6 @@ async function onFailure({
syncJobId?: number | undefined;
syncName: string;
providerConfigKey: string;
activityLogId: string;
runTime: number;
error: NangoError;
}): Promise<void> {
Expand All @@ -219,8 +213,6 @@ async function onFailure({
createdAt: Date.now()
});
}
const logCtx = await logContextGetter.get({ id: activityLogId });
await logCtx.error(error.message, { error });

if (syncJobId) {
await updateSyncJobStatus(syncJobId, SyncStatus.STOPPED);
Expand Down
15 changes: 2 additions & 13 deletions packages/server/lib/controllers/proxy.controller.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Request, Response, NextFunction } from 'express';
import type { OutgoingHttpHeaders, IncomingHttpHeaders } from 'http';
import type { OutgoingHttpHeaders } from 'http';
import type { TransformCallback } from 'stream';
import type stream from 'stream';
import { Readable, Transform, PassThrough } from 'stream';
Expand All @@ -10,7 +10,7 @@ import type { AxiosError, AxiosRequestConfig, AxiosResponse } from 'axios';
import { backOff } from 'exponential-backoff';
import type { HTTP_VERB, UserProvidedProxyConfiguration, InternalProxyConfiguration, ApplicationConstructedProxyConfiguration } from '@nangohq/shared';
import { NangoError, LogActionEnum, errorManager, ErrorSourceEnum, proxyService, connectionService, configService, featureFlags } from '@nangohq/shared';
import { metrics, getLogger, axiosInstance as axios } from '@nangohq/utils';
import { metrics, getLogger, axiosInstance as axios, getHeaders } from '@nangohq/utils';
import { logContextGetter } from '@nangohq/logs';
import { connectionRefreshFailed as connectionRefreshFailedHook, connectionRefreshSuccess as connectionRefreshSuccessHook } from '../hooks/hooks.js';
import type { LogContext } from '@nangohq/logs';
Expand Down Expand Up @@ -162,17 +162,6 @@ class ProxyController {
metrics.increment(metrics.Types.PROXY_FAILURE);
next(err);
} finally {
const getHeaders = (hs: IncomingHttpHeaders | OutgoingHttpHeaders): Record<string, string> => {
const headers: Record<string, string> = {};
for (const [key, value] of Object.entries(hs)) {
if (typeof value === 'string') {
headers[key] = value;
} else if (Array.isArray(value)) {
headers[key] = value.join(', ');
}
}
return headers;
};
const reqHeaders = getHeaders(req.headers);
reqHeaders['authorization'] = 'REDACTED';
await logCtx?.enrichOperation({
Expand Down
17 changes: 15 additions & 2 deletions packages/server/lib/controllers/sync.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
import type { LogContext } from '@nangohq/logs';
import { defaultOperationExpiration, logContextGetter } from '@nangohq/logs';
import type { LastAction } from '@nangohq/records';
import { isHosted } from '@nangohq/utils';
import { getHeaders, isHosted } from '@nangohq/utils';
import { records as recordsService } from '@nangohq/records';
import type { RequestLocals } from '../utils/express.js';
import { getOrchestrator } from '../utils/utils.js';
Expand Down Expand Up @@ -324,7 +324,6 @@ class SyncController {
return;
} else {
span.setTag('nango.error', actionResponse.error);
await logCtx.error('Failed to run action', { error: actionResponse.error });
await logCtx.failed();

errorManager.errResFromNangoErr(res, actionResponse.error);
Expand All @@ -340,6 +339,20 @@ class SyncController {
}

next(err);
} finally {
const reqHeaders = getHeaders(req.headers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

On your other PR you also need this + the redacted
Maybe you should consolidate everything with this new function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes that's my goal

reqHeaders['authorization'] = 'REDACTED';
await logCtx?.enrichOperation({
request: {
url: `${req.protocol}://${req.get('host')}${req.originalUrl}`,
method: req.method,
headers: reqHeaders
},
response: {
code: res.statusCode,
headers: getHeaders(res.getHeaders())
}
});
}
}

Expand Down
Loading
Loading