From 8fc1095d1e4259e263fcba4467e3e819541d0555 Mon Sep 17 00:00:00 2001 From: Ahsan Virani Date: Thu, 24 Feb 2022 17:15:30 +0100 Subject: [PATCH] :bug: Swallow telemetry error and only log in warn and debug mode (#2858) * catch nodegraph errors * use loglevel config for telemetry * Use getByNameAndVersion instead of getByName * remove any usage of nodeTypes.getByName method * deprecate getByName method --- packages/cli/src/CredentialsHelper.ts | 9 +- packages/cli/src/NodeTypes.ts | 7 -- packages/cli/src/telemetry/index.ts | 3 +- .../src/components/mixins/workflowHelpers.ts | 12 --- packages/workflow/src/Interfaces.ts | 1 - packages/workflow/src/TelemetryHelpers.ts | 93 ++++++++++--------- packages/workflow/test/Helpers.ts | 3 +- packages/workflow/test/RoutingNode.test.ts | 4 +- 8 files changed, 58 insertions(+), 74 deletions(-) diff --git a/packages/cli/src/CredentialsHelper.ts b/packages/cli/src/CredentialsHelper.ts index a0b52c680f..497894254d 100644 --- a/packages/cli/src/CredentialsHelper.ts +++ b/packages/cli/src/CredentialsHelper.ts @@ -54,13 +54,6 @@ const mockNodeTypes: INodeTypes = { // @ts-ignore return Object.values(this.nodeTypes).map((data) => data.type); }, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - getByName(nodeType: string): INodeType | INodeVersionedType | undefined { - if (this.nodeTypes[nodeType] === undefined) { - return undefined; - } - return this.nodeTypes[nodeType].type; - }, getByNameAndVersion(nodeType: string, version?: number): INodeType | undefined { if (this.nodeTypes[nodeType] === undefined) { return undefined; @@ -524,7 +517,7 @@ export class CredentialsHelper extends ICredentialsHelper { nodeType = credentialTestFunction.nodeType; } else { const nodeTypes = NodeTypes(); - nodeType = nodeTypes.getByName('n8n-nodes-base.noOp') as INodeType; + nodeType = nodeTypes.getByNameAndVersion('n8n-nodes-base.noOp'); } const node: INode = { diff --git a/packages/cli/src/NodeTypes.ts b/packages/cli/src/NodeTypes.ts index 046123927b..3667059dce 100644 --- a/packages/cli/src/NodeTypes.ts +++ b/packages/cli/src/NodeTypes.ts @@ -33,13 +33,6 @@ class NodeTypesClass implements INodeTypes { return Object.values(this.nodeTypes).map((data) => data.type); } - getByName(nodeType: string): INodeType | INodeVersionedType | undefined { - if (this.nodeTypes[nodeType] === undefined) { - throw new Error(`The node-type "${nodeType}" is not known!`); - } - return this.nodeTypes[nodeType].type; - } - /** * Variant of `getByNameAndVersion` that includes the node's source path, used to locate a node's translations. */ diff --git a/packages/cli/src/telemetry/index.ts b/packages/cli/src/telemetry/index.ts index fb4f53460d..6b678f099f 100644 --- a/packages/cli/src/telemetry/index.ts +++ b/packages/cli/src/telemetry/index.ts @@ -58,6 +58,7 @@ export class Telemetry { this.versionCli = versionCli; const enabled = config.get('diagnostics.enabled') as boolean; + const logLevel = config.get('logs.level') as boolean; if (enabled) { const conf = config.get('diagnostics.config.backend') as string; const [key, url] = conf.split(';'); @@ -69,7 +70,7 @@ export class Telemetry { return; } - this.client = new TelemetryClient(key, url); + this.client = new TelemetryClient(key, url, { logLevel }); this.pulseIntervalReference = setInterval(async () => { void this.pulse(); diff --git a/packages/editor-ui/src/components/mixins/workflowHelpers.ts b/packages/editor-ui/src/components/mixins/workflowHelpers.ts index 6bb698650c..2e6644a54e 100644 --- a/packages/editor-ui/src/components/mixins/workflowHelpers.ts +++ b/packages/editor-ui/src/components/mixins/workflowHelpers.ts @@ -24,7 +24,6 @@ import { IRunExecutionData, IWorfklowIssues, IWorkflowDataProxyAdditionalKeys, - TelemetryHelpers, Workflow, NodeHelpers, } from 'n8n-workflow'; @@ -224,17 +223,6 @@ export const workflowHelpers = mixins( // Does not get used in Workflow so no need to return it return []; }, - getByName: (nodeType: string): INodeType | INodeVersionedType | undefined => { - const nodeTypeDescription = this.$store.getters.nodeType(nodeType) as INodeTypeDescription | null; - - if (nodeTypeDescription === null) { - return undefined; - } - - return { - description: nodeTypeDescription, - }; - }, getByNameAndVersion: (nodeType: string, version?: number): INodeType | undefined => { const nodeTypeDescription = this.$store.getters.nodeType(nodeType, version) as INodeTypeDescription | null; diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index fa0c687265..8a5b04a4bd 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -1193,7 +1193,6 @@ export interface INodeTypes { nodeTypes: INodeTypeData; init(nodeTypes?: INodeTypeData): Promise; getAll(): Array; - getByName(nodeType: string): INodeType | INodeVersionedType | undefined; getByNameAndVersion(nodeType: string, version?: number): INodeType | undefined; } diff --git a/packages/workflow/src/TelemetryHelpers.ts b/packages/workflow/src/TelemetryHelpers.ts index 33d71fea20..04499bb375 100644 --- a/packages/workflow/src/TelemetryHelpers.ts +++ b/packages/workflow/src/TelemetryHelpers.ts @@ -8,9 +8,10 @@ import { INodesGraphResult, IWorkflowBase, INodeTypes, - INodeType, } from '.'; +import { getInstance as getLoggerInstance } from './LoggerProxy'; + export function getNodeTypeForName(workflow: IWorkflowBase, nodeName: string): INode | undefined { return workflow.nodes.find((node) => node.name === nodeName); } @@ -26,51 +27,59 @@ export function generateNodesGraph( }; const nodeNameAndIndex: INodeNameIndex = {}; - workflow.nodes.forEach((node: INode, index: number) => { - nodesGraph.node_types.push(node.type); - const nodeItem: INodeGraphItem = { - type: node.type, - position: node.position, + try { + workflow.nodes.forEach((node: INode, index: number) => { + nodesGraph.node_types.push(node.type); + const nodeItem: INodeGraphItem = { + type: node.type, + position: node.position, + }; + + if (node.type === 'n8n-nodes-base.httpRequest') { + try { + nodeItem.domain = new URL(node.parameters.url as string).hostname; + } catch (e) { + nodeItem.domain = node.parameters.url as string; + } + } else { + const nodeType = nodeTypes.getByNameAndVersion(node.type); + + nodeType?.description.properties.forEach((property) => { + if ( + property.name === 'operation' || + property.name === 'resource' || + property.name === 'mode' + ) { + nodeItem[property.name] = property.default ? property.default.toString() : undefined; + } + }); + + nodeItem.operation = node.parameters.operation?.toString() ?? nodeItem.operation; + nodeItem.resource = node.parameters.resource?.toString() ?? nodeItem.resource; + nodeItem.mode = node.parameters.mode?.toString() ?? nodeItem.mode; + } + nodesGraph.nodes[`${index}`] = nodeItem; + nodeNameAndIndex[node.name] = index.toString(); + }); + + const getGraphConnectionItem = (startNode: string, connectionItem: IConnection) => { + return { start: nodeNameAndIndex[startNode], end: nodeNameAndIndex[connectionItem.node] }; }; - if (node.type === 'n8n-nodes-base.httpRequest') { - try { - nodeItem.domain = new URL(node.parameters.url as string).hostname; - } catch (e) { - nodeItem.domain = node.parameters.url as string; - } - } else { - const nodeType = nodeTypes.getByName(node.type) as INodeType; - nodeType.description.properties.forEach((property) => { - if ( - property.name === 'operation' || - property.name === 'resource' || - property.name === 'mode' - ) { - nodeItem[property.name] = property.default ? property.default.toString() : undefined; - } - }); - - nodeItem.operation = node.parameters.operation?.toString() ?? nodeItem.operation; - nodeItem.resource = node.parameters.resource?.toString() ?? nodeItem.resource; - nodeItem.mode = node.parameters.mode?.toString() ?? nodeItem.mode; - } - nodesGraph.nodes[`${index}`] = nodeItem; - nodeNameAndIndex[node.name] = index.toString(); - }); - - const getGraphConnectionItem = (startNode: string, connectionItem: IConnection) => { - return { start: nodeNameAndIndex[startNode], end: nodeNameAndIndex[connectionItem.node] }; - }; - - Object.keys(workflow.connections).forEach((nodeName) => { - const connections = workflow.connections[nodeName]; - connections.main.forEach((element) => { - element.forEach((element2) => { - nodesGraph.node_connections.push(getGraphConnectionItem(nodeName, element2)); + Object.keys(workflow.connections).forEach((nodeName) => { + const connections = workflow.connections[nodeName]; + connections.main.forEach((element) => { + element.forEach((element2) => { + nodesGraph.node_connections.push(getGraphConnectionItem(nodeName, element2)); + }); }); }); - }); + } catch (e) { + const logger = getLoggerInstance(); + logger.warn(`Failed to generate nodes graph for workflowId: ${workflow.id as string | number}`); + logger.warn((e as Error).message); + logger.warn((e as Error).stack ?? ''); + } return { nodeGraph: nodesGraph, nameIndices: nodeNameAndIndex }; } diff --git a/packages/workflow/test/Helpers.ts b/packages/workflow/test/Helpers.ts index 7c68850479..103513283b 100644 --- a/packages/workflow/test/Helpers.ts +++ b/packages/workflow/test/Helpers.ts @@ -23,6 +23,7 @@ import { INodeType, INodeTypeData, INodeTypes, + INodeVersionedType, IRunExecutionData, ITaskDataConnections, IWorkflowBase, @@ -614,7 +615,7 @@ class NodeTypesClass implements INodeTypes { return Object.values(this.nodeTypes).map((data) => NodeHelpers.getVersionedNodeType(data.type)); } - getByName(nodeType: string): INodeType { + getByName(nodeType: string): INodeType | INodeVersionedType | undefined { return this.getByNameAndVersion(nodeType); } diff --git a/packages/workflow/test/RoutingNode.test.ts b/packages/workflow/test/RoutingNode.test.ts index 33d7d3123c..2ead2e5bfe 100644 --- a/packages/workflow/test/RoutingNode.test.ts +++ b/packages/workflow/test/RoutingNode.test.ts @@ -618,7 +618,7 @@ describe('RoutingNode', () => { const runExecutionData: IRunExecutionData = { resultData: { runData: {} } }; const additionalData = Helpers.WorkflowExecuteAdditionalData(); const path = ''; - const nodeType = nodeTypes.getByName(node.type); + const nodeType = nodeTypes.getByNameAndVersion(node.type); const workflowData = { nodes: [node], @@ -1596,7 +1596,7 @@ describe('RoutingNode', () => { const connectionInputData: INodeExecutionData[] = []; const runExecutionData: IRunExecutionData = { resultData: { runData: {} } }; const additionalData = Helpers.WorkflowExecuteAdditionalData(); - const nodeType = nodeTypes.getByName(baseNode.type); + const nodeType = nodeTypes.getByNameAndVersion(baseNode.type); const inputData: ITaskDataConnections = { main: [