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

Automation logging updates #10626

Merged
merged 5 commits into from
May 17, 2023
Merged

Conversation

Rory-Powell
Copy link
Contributor

Description

Updates to logging for better pino integration to aid debugging via datadog.

Additionally add some better type definitions around bull jobs specific to automations.

@Rory-Powell Rory-Powell requested a review from mike12345567 May 17, 2023 12:58
// An error occurred.
console.error(...getLogParams(eventType, BullEvent.ERROR, { error }))
})

if (process.env.NODE_DEBUG?.includes("bull")) {
Copy link
Contributor Author

@Rory-Powell Rory-Powell May 17, 2023

Choose a reason for hiding this comment

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

I'm planning on turning off bull debug logging by default (after investigation) to bring down logging costs, hence the stalled and error logs above being promoted to logging by default

Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

LGTM! Would be good after getting this merged to pull it back to development as some of the types changes will have implications for sync automations - theres a few changes around Job types already!

Copy link
Collaborator

@adrinr adrinr left a comment

Choose a reason for hiding this comment

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

Small comment, but LGTM

@@ -104,6 +104,22 @@ async function newContext(updates: ContextMap, task: any) {
return Context.run(context, task)
}

export async function doInAutomationContext(
appId: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use an object as a parameter to make it more verbose in its usage?

export async function doInAutomationContext(params: {
  appId: string,
  automationId: string,
  task: any
})

@PClmnt
Copy link
Collaborator

PClmnt commented May 17, 2023

LGTM! Would be good after getting this merged to pull it back to development as some of the types changes will have implications for sync automations - theres a few changes around Job types already!

+1 to this, some of the stuff you've done around the triggers and automation thread code overlap quite heavily with some work I was doing for sync automations. LGTM though!

@Rory-Powell Rory-Powell merged commit 5f72135 into master May 17, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2023
@Rory-Powell Rory-Powell deleted the chore/automation-logging-updates branch May 17, 2023 14:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants