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

feat(logs): timeout old operations #2220

Merged
merged 21 commits into from
Jun 4, 2024
Merged

Conversation

bodinsamuel
Copy link
Collaborator

@bodinsamuel bodinsamuel commented May 29, 2024

Describe your changes

Fixes NAN-1036

Since we have only 15days retention and syncs are unlimited it's mostly for cosmetic and some smaller better time scoped operations (action, oauth, webhook).

  • Add expiration date to all operations
  • Cron to set timeout to all expired operations

How to test?

  • Trigger an oauth but don't complete the process and wait 5-10minutes
    or
  • Add an operation, change the expiresAt manually, wait for the cron

(The query in curl)

#!/usr/bin/env bash

curl -X POST "http://localhost:55132/20240528_messages.2024-05-30/_search" -H 'Content-Type: application/json' -d'
{
  "query": {
            "bool": {
                "must": [{ "range": { "expiresAt": { "lt": "now" } } }],
                "must_not": { "exists": { "field": "parentId" } },
                "should": [{ "term": { "state": "waiting" } }, { "term": { "state": "running" } }]
            }
        }
}
'

@bodinsamuel bodinsamuel self-assigned this May 29, 2024
Base automatically changed from feat/logs-index-policy to master May 30, 2024 07:50
Copy link

linear bot commented May 30, 2024

@bodinsamuel bodinsamuel marked this pull request as ready for review May 30, 2024 16:00
startedAt: data.startedAt || null,
endedAt: data.endedAt || null
endedAt: data.endedAt || null,
expiresAt: data.operation ? data.expiresAt || new Date(now.getTime() + 7 * 86400 * 1000).toISOString() : null
Copy link
Collaborator

Choose a reason for hiding this comment

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

why 7 days by default. Can any operation last longer than 24hours?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

technically yes I believe, syncs can run as long as they need if the heartbeat works no?

Copy link
Member

Choose a reason for hiding this comment

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

syncs can run as long as they need if the heartbeat works no?

A sync can't run longer than 24 hours https://github.com/NangoHQ/nango/blob/master/packages/jobs/lib/workflows.ts#L6

operation: { type: 'action' },
message: 'Start action',
expiresAt: new Date(Date.now() + 86400 * 1000).toISOString()
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

1 day for an action that's pretty generous

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's just for the heartbeat, unfortunately I have no way to have something more precise. But if everything works well everything should be handled by temporal/v2 :D

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to abstract out these times to one place in case we need to adjust

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah good point, actually I wasn't aware of the constant you shared on previous message so maybe I can expose them globally somehow

Copy link

linear bot commented May 31, 2024

id: String(activityLogId),
operation: { type: 'auth', action: 'create_connection' },
message: 'Authorization OAuth',
expiresAt: new Date(Date.now() + 300 * 1000).toISOString()
Copy link
Member

Choose a reason for hiding this comment

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

Feel like this expiresAt should be a constant somewhere since it is repeated in so many places

new Date(Date.now() + 300 * 1000).toISOString()

Copy link
Member

@khaliqgant khaliqgant left a comment

Choose a reason for hiding this comment

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

Abstraction request

@bodinsamuel
Copy link
Collaborator Author

Abstracted the values, I had to expose them through utils let me know.
(could also be helpful for you thomas)


export const WEBHOOK_TIMEOUT = '15m';
export const WEBHOOK_MAX_ATTEMPTS = 3;
export const MAX_WEBHOOK_DURATION = ms(WEBHOOK_TIMEOUT);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note I didn't removed them from jobs/workflows.ts because temporal can't compile utils 🤦🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine. I just have to remember to remove it when getting rid of temporal

@bodinsamuel
Copy link
Collaborator Author

Okay this time should be fine, I could not replace webhooks timing because I can't import logs in shared

Copy link
Member

@khaliqgant khaliqgant left a comment

Choose a reason for hiding this comment

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

Thanks

@bodinsamuel bodinsamuel enabled auto-merge (squash) June 4, 2024 08:11
@bodinsamuel bodinsamuel merged commit 856d710 into master Jun 4, 2024
20 checks passed
@bodinsamuel bodinsamuel deleted the feat/logs-automatic-timeout branch June 4, 2024 08:16
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.

3 participants