Skip to content

fix(node): Make sure we use same ID for checkIns #8050

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

Merged
merged 4 commits into from
May 8, 2023

Conversation

AbhiPrasad
Copy link
Member

Based on work in #8039, this PR fixes a bug where we were adding a unique ID to every check in, which is incorrect behaviour. Instead we now return a checkInId every time a check in is captured, which can be used to link check ins together.

Example:

// 🟡 Notify Sentry your job is running:
const checkInId = Sentry.captureCheckIn({
  monitorSlug: '<monitor-slug>',
  status: 'in_progress',
});

// Execute your scheduled task here...

// 🟢 Notify Sentry your job has completed successfully:
Sentry.captureCheckIn({
  checkInId,
  monitorSlug: '<monitor-slug>',
  status: 'ok',
});

@AbhiPrasad AbhiPrasad requested a review from lforst May 5, 2023 07:21
// The distinct slug of the monitor.
monitorSlug: SerializedCheckIn['monitor_slug'];
// Check-In ID (unique and client generated).
checkInId?: SerializedCheckIn['check_in_id'];
Copy link
Contributor

Choose a reason for hiding this comment

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

You could theoretically call captureCheckIn with status "in_progress" but not provide a checkInId. We should couple those types. Not coupling would be very opaque for users why things are not working.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup refined with 33fd221

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that commit is on the wrong branch

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦

// The distinct slug of the monitor.
monitorSlug: SerializedCheckIn['monitor_slug'];
// The status of the check-in.
status: 'ok' | 'error';
// Check-In ID (unique and client generated).
checkInId?: SerializedCheckIn['check_in_id'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to make the ID required for FinishedCheckIns?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, why the hell do we need both an ID and a name??

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to make the ID required for FinishedCheckIns

It makes the DX worse, so no. This is because technically an ID can be undefined if the client is disabled (so the first captureCheckIn will return undefined in that case)

Honestly, why the hell do we need both an ID and a name??

It's because you can have multiple crons with the same name fire at the same time, the id helps make sure that this works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to explain the point I was making here because I think you misunderstood me here: For the finished checkins we theoretically don't need to send the id AND the slug since we can already map from id to slug in the product.

I still stand by my point that we should not be required to send both but I'll approve this to unblock so that our current implementation is not busted.

@AbhiPrasad AbhiPrasad self-assigned this May 5, 2023
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) May 8, 2023 08:13
@AbhiPrasad AbhiPrasad merged commit 4588954 into develop May 8, 2023
@AbhiPrasad AbhiPrasad deleted the abhi-cron-return-value branch May 8, 2023 08:20
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