Skip to content

Conversation

WillieRuemmele
Copy link
Contributor

What does this PR do?

removes the process.on('UncaughtException' listener that was catching all of our errors, and then node was not re-throwing them, so we were missing information in our telemetry

refactors a command's message doc to share names

What issues does this PR fix or reference?

@W-15887232@

@WillieRuemmele WillieRuemmele requested a review from a team as a code owner June 3, 2024 22:17
// process.on('uncaughtException', err => {
throw SfError.wrap(messages.createError('apexLibErr', [(e as Error).message]));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

the try/catch wrapper can be removed, sfCommand.catch handles all errors here:
https://github.com/salesforcecli/sf-plugins-core/blob/main/src/sfCommand.ts#L402C1-L408C7

what if we ask apex-node to use SfError instead of catching everything here?

https://github.com/search?q=repo%3Aforcedotcom%2Fsalesforcedx-apex+%2Fthrow+%28new+Error%7CError%29%5C%28%2F&type=code

that way the CLI get better errors too (IDEx team can add error.actions if applicable, we preserve their original error.name,etc).

// what used to be caught by the
// process.on('uncaughtException', err => {
throw SfError.wrap(messages.createError('apexLibErr', [(e as Error).message]));
}
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

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

try/catch blocks can be removed, sfError will handle these and keep the generic error.name from apex-node.

@cristiand391
Copy link
Member

QA notes:

setup: plugin compiled and linked into sf
used profile without access to apex stuff

403 err:

➜  plugin-apex git:(wr/removeUncaughtListeners) ✗ sf apex run test --tests GeocodingServiceTest --wait 2 -o 1717697903578_test-kbf0ic8olbbs@example.com
 ›   Warning: @salesforce/plugin-apex is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used instead.
/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/@salesforce/apex-node/lib/src/streaming/streamingClient.js:101
                    throw new Error(message.error);
                          ^

Error: 403::User not enabled for streaming
    at Object.incoming (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/@salesforce/apex-node/lib/src/streaming/streamingClient.js:101:27)
    at pipe (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/protocol/extensible.js:39:43)
    at klass.pipeThroughExtensions (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/protocol/extensible.js:41:5)
    at klass._receiveMessage (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/protocol/client.js:345:10)
    at klass.handler (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/mixins/publisher.js:13:41)
    at EventEmitter.emit (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/util/event_emitter.js:65:17)
    at klass.handleResponse (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/protocol/dispatcher.js:140:10)
    at klass._receive (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/transport/transport.js:102:24)
    at IncomingMessage.<anonymous> (/Users/cdominguez/code/gh/sf/plugin-apex/node_modules/faye/src/transport/node_http.js:126:14)
    at IncomingMessage.emit (node:events:526:35)

✅ command execution is included in telemetry:

SF_TELEMETRY_DEBUG=true SF_DISABLE_TELEMETRY=false sf apex run test --tests GeocodingServiceTest --wait 2 -o 1717697903578_test-kbf0ic8olbbs@example.com --dev-debug
...
![Screenshot 2024-06-07 at 3 02 34 PM](https://github.com/salesforcecli/plugin-apex/assets/6853656/89b50f15-8af5-4cbf-81f9-46cbce9739f4)

❌ command error isn't included in telemetry:

the telemetry payload only includes the command exec info:
Screenshot 2024-06-07 at 3 04 19 PM
I see that sfCommand.catch never gets executed so it never emits the err telemetry info.

following this comment:
faye/faye#411 (comment)

I tested defining outgoing and throwing from the in the faye client in apex-node and the error is properly handled, I'll get a WI for IDEx team to handle these in apex-node.

@cristiand391 cristiand391 merged commit 4059333 into main Jun 7, 2024
@cristiand391 cristiand391 deleted the wr/removeUncaughtListeners branch June 7, 2024 18:29
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.

2 participants