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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf410ad
fix(node): Make sure we use same ID for checkIns
AbhiPrasad May 4, 2023
632a9d0
make types more strict
AbhiPrasad May 5, 2023
91b28a6
actually get the type correct this time
AbhiPrasad May 5, 2023
b12db4b
always return an id from capture methods
AbhiPrasad May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions packages/node/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
* @param checkIn An object that describes a check in.
* @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want
* to create a monitor automatically when sending a check in.
* @returns A string representing the id of the check in.
*/
public captureCheckIn(checkIn: CheckIn, monitorConfig?: MonitorConfig): void {
public captureCheckIn(checkIn: CheckIn, monitorConfig?: MonitorConfig): string | undefined {
if (!this._isEnabled()) {
__DEBUG_BUILD__ && logger.warn('SDK not enabled, will not capture checkin.');
return;
Expand All @@ -163,15 +164,20 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
const options = this.getOptions();
const { release, environment, tunnel } = options;

const id = checkIn.status === 'in_progress' ? uuid4() : checkIn.checkInId;

const serializedCheckIn: SerializedCheckIn = {
check_in_id: uuid4(),
check_in_id: id,
monitor_slug: checkIn.monitorSlug,
status: checkIn.status,
duration: checkIn.duration,
release,
environment,
};

if (checkIn.status !== 'in_progress') {
serializedCheckIn.duration = checkIn.duration;
}

if (monitorConfig) {
serializedCheckIn.monitor_config = {
schedule: monitorConfig.schedule,
Expand All @@ -183,6 +189,7 @@ export class NodeClient extends BaseClient<NodeClientOptions> {

const envelope = createCheckInEnvelope(serializedCheckIn, this.getSdkMetadata(), tunnel, this.getDsn());
void this._sendEnvelope(envelope);
return id;
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/node/src/sdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ export function captureCheckIn(
}

__DEBUG_BUILD__ && logger.warn('Cannot capture check in. No client defined.');
return;
}

/** Node.js stack parser */
Expand Down
29 changes: 24 additions & 5 deletions packages/node/test/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ describe('NodeClient', () => {
// @ts-ignore accessing private method
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');

client.captureCheckIn(
{ monitorSlug: 'foo', status: 'ok', duration: 1222 },
const id = client.captureCheckIn(
{ monitorSlug: 'foo', status: 'in_progress' },
{
schedule: {
type: 'crontab',
Expand All @@ -315,9 +315,8 @@ describe('NodeClient', () => {
expect.any(Object),
{
check_in_id: expect.any(String),
duration: 1222,
monitor_slug: 'foo',
status: 'ok',
status: 'in_progress',
release: '1.0.0',
environment: 'dev',
monitor_config: {
Expand All @@ -333,6 +332,26 @@ describe('NodeClient', () => {
],
],
]);

client.captureCheckIn({ monitorSlug: 'foo', status: 'ok', duration: 1222, checkInId: id });

expect(sendEnvelopeSpy).toHaveBeenCalledTimes(2);
expect(sendEnvelopeSpy).toHaveBeenCalledWith([
expect.any(Object),
[
[
expect.any(Object),
{
check_in_id: id,
monitor_slug: 'foo',
duration: 1222,
status: 'ok',
release: '1.0.0',
environment: 'dev',
},
],
],
]);
});

it('does not send a checkIn envelope if disabled', () => {
Expand All @@ -342,7 +361,7 @@ describe('NodeClient', () => {
// @ts-ignore accessing private method
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');

client.captureCheckIn({ monitorSlug: 'foo', status: 'ok', duration: 1222 });
client.captureCheckIn({ monitorSlug: 'foo', status: 'in_progress' });

expect(sendEnvelopeSpy).toHaveBeenCalledTimes(0);
});
Expand Down
15 changes: 13 additions & 2 deletions packages/types/src/checkin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,26 @@ export interface SerializedCheckIn {
};
}

export interface CheckIn {
interface InProgressCheckIn {
// The distinct slug of the monitor.
monitorSlug: SerializedCheckIn['monitor_slug'];
// The status of the check-in.
status: SerializedCheckIn['status'];
status: 'in_progress';
}

interface FinishedCheckIn {
// 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.

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.

🤦

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.

// The duration of the check-in in seconds. Will only take effect if the status is ok or error.
duration?: SerializedCheckIn['duration'];
}

export type CheckIn = InProgressCheckIn | FinishedCheckIn;

type SerializedMonitorConfig = NonNullable<SerializedCheckIn['monitor_config']>;

export interface MonitorConfig {
Expand Down