feat: implement notifications in error handling logic #52
Conversation
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
jahabeebs
force-pushed
the
feat/grt-60-error-handling-2
branch
from
1437553 to
7ac5c81
Compare
| constructor(config: DiscordNotifierConfig) { | ||
| const intents = new IntentsBitField().add( | ||
| IntentsBitField.Flags.Guilds, | ||
| IntentsBitField.Flags.GuildMessages, | ||
| ); | ||
| this.client = new Client({ intents }); | ||
| this.config = config; | ||
| this.readyPromise = this.initialize(); | ||
| } | ||
|
|
||
| /** | ||
| * Initializes the Discord notifier by logging in with the bot token and waiting for the "ready" event. | ||
| * @returns {Promise<void>} A promise that resolves when the Discord bot is ready. | ||
| * @throws {Error} If the initialization fails. | ||
| */ | ||
| private async initialize(): Promise<void> { | ||
| try { | ||
| await this.client.login(this.config.discordBotToken); | ||
| await new Promise<void>((resolve) => { | ||
| this.client.once("ready", () => { | ||
| console.log("Discord bot is ready"); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to initialize Discord notifier:", error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
here we can use the Static Async Factory approach from the Notion docs:
There was a problem hiding this comment.
To get around context any I suppose we would have to define a context type for each error (or classify them into groups) but I think this might take a while--let's see what the others think
There was a problem hiding this comment.
Let's standardize the context to:
{
request: Request,
response?: Response,
dispute?: Dispute,
event?: EboEvent<EboEventName>,
registry: EboRegistry
}This should be enough for our use cases.
There was a problem hiding this comment.
Ok, I think I need to do this in the error handling PR rather than this notification PR though
| discordBotToken: process.env.DISCORD_BOT_TOKEN!, | ||
| discordChannelId: process.env.DISCORD_CHANNEL_ID!, |
There was a problem hiding this comment.
instead of retrieving it from env, it's better from a config or env object previously validated with Zod
the notifier is kinda like the logger, so injecting it as argument makes sense, wdyt?
There was a problem hiding this comment.
DI is the way to go here, I agree!
There was a problem hiding this comment.
Refactored to use DI and zod
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
Signed-off-by: jahabeebs <[email protected]>
jahabeebs
force-pushed
the
feat/grt-59-implement-notifications
branch
from
a73d337 to
e03e697
Compare
apps/agent/src/config/index.ts
Outdated
| DISCORD_BOT_TOKEN: envData.DISCORD_BOT_TOKEN || "", | ||
| DISCORD_CHANNEL_ID: envData.DISCORD_CHANNEL_ID || "", |
There was a problem hiding this comment.
what does the empty string mean on Discord configuration?
There was a problem hiding this comment.
this was because of type issues that I had before implementing zod but now it's not needed so I deleted--thanks for pointing it out
| private formatErrorMessage(error: Error, context: any): string { | ||
| return `**Error:** ${error.name} - ${error.message}\n**Context:**\n\`\`\`json\n${JSON.stringify( | ||
| context, | ||
| null, | ||
| 2, | ||
| )}\n\`\`\``; | ||
| } |
There was a problem hiding this comment.
idk if context could have a bigint field, but it will break since JSON is not compatible natively with bigints
two alternatives:
- write a wrapper of stringify and manually handle bigint cases
- use
stringifyfrom Viem which already handles this case
There was a problem hiding this comment.
That's a super nice catch.
Apparently there's a way to do this "natively" by just using a second param of JSON.stringify. This SO answer has more info about it.
There was a problem hiding this comment.
great catch, switched it out with viem's stringify. I checked out the stack overflow link but it seems like using the native JSON.stringify requires intermediate types and I think it would be best just to use viem's helper method for this
…notifications # Conflicts: # packages/automated-dispute/src/exceptions/customContractError.ts # packages/automated-dispute/src/exceptions/errorFactory.ts # packages/automated-dispute/src/exceptions/errorHandler.ts # packages/automated-dispute/src/exceptions/index.ts # packages/automated-dispute/src/providers/protocolProvider.ts # packages/automated-dispute/src/services/eboActor.ts # packages/automated-dispute/src/services/index.ts # packages/automated-dispute/src/types/errorTypes.ts # packages/automated-dispute/tests/exceptions/errorFactory.spec.ts # packages/automated-dispute/tests/services/eboActor.spec.ts # packages/automated-dispute/tests/services/eboActor/onResponseProposed.spec.ts # pnpm-lock.yaml
|
@0xyaco I fixed the merge conflicts here and kind of had to redo most of the notification logic because in the error handling issue we changed where we do the notification logic...so here's a brief summary of the changes
There are some strange test errors happening with eboProcess.spec.ts--I've been digging around seeing if I missed a mock somewhere |
| * @param {DiscordNotifierConfig} config - The configuration object for the DiscordNotifier. | ||
| * @returns {Promise<DiscordNotifier>} A promise that resolves to a DiscordNotifier instance. | ||
| */ | ||
| public static async create(config: DiscordNotifierConfig): Promise<DiscordNotifier> { |
There was a problem hiding this comment.
Let's pass an ILogger param here to DI a logger instead of using console.
| this.notificationService = notificationService; | ||
| } | ||
|
|
||
| public async handle(error: CustomContractError, logger: ILogger): Promise<void> { |
There was a problem hiding this comment.
I think we can drop the ILogger param here and put it in the constructor.
| console.log("Error notification sent to Discord"); | ||
| } catch (err) { | ||
| console.error("Failed to send error notification to Discord:", err); | ||
| throw err; |
There was a problem hiding this comment.
Let's not throw here for the moment, just error logging needed.
Ideally we should have redundant ways of notifying (like Discord, SMS, email, etc.); but for the moment, we don't want the whole agent to fail if Discord's API is down + there are third party monitoring solutions that are designed to do this so.. In case the operator needs more notification redundancy, they can always install some of those tools.
|
@0xyaco Implemented your suggestions and fixed the tests ✅
Also the reason the test was throwing that weird error was because of a serialization error in an expect() statement so I broke up the expect statement more to avoid the issue |
| public async notifyError(error: Error, context: any): Promise<void> { | ||
| try { | ||
| const channel = await this.client.channels.fetch(this.config.discordChannelId); | ||
| if (!channel || !channel.isTextBased()) { | ||
| throw new Error("Discord channel not found or is not text-based"); | ||
| } | ||
| const errorMessage = this.formatErrorMessage(error, context); | ||
| await (channel as TextChannel).send(errorMessage); | ||
| this.logger.info("Error notification sent to Discord"); | ||
| } catch (err) { | ||
| this.logger.error(`Failed to send error notification to Discord: ${err}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Formats the error message to be sent to Discord. | ||
| * @param {Error} error - The error object. | ||
| * @param {any} context - Additional context information. | ||
| * @returns {string} The formatted error message. | ||
| */ | ||
| private formatErrorMessage(error: Error, context: any): string { |
There was a problem hiding this comment.
typing context as unknown instead of any
…notifications # Conflicts: # packages/automated-dispute/tests/mocks/eboActor.mocks.ts # packages/automated-dispute/tests/services/eboProcessor.spec.ts
🤖 Linear
Closes GRT-59
Description