feat(transport): Return result through Transport send

[timfish]

For it to be possible to wrap the transport for offline support (#6403), wrapping transports need to be able to cater for rate limiting and therefore send needs to return TransportMakeRequestResponse rather than void. To move in this direction while minimising breaking changes, this PR changes the return to Promise<void | TransportMakeRequestResponse>.

This PR changes the logic so that send now throws in failure cases. This is not a concern for internal code because the two places it's called errors are caught and logged:

sentry-javascript/packages/core/src/baseclient.ts

Lines 739 to 741 in 36488ec

	this._transport.send(envelope).then(null, reason => {
	__DEBUG_BUILD__ && logger.error('Error while sending event:', reason);
	});

sentry-javascript/packages/replay/src/replay.ts

Lines 994 to 998 in 371d42d

	try {
	return await transport.send(envelope);
	} catch {
	throw new Error(UNABLE_TO_SEND_REPLAY);
	}

The only real risk I see here is if customers are manually calling transport.send and have come to expect that it never throws.

[Lms24]

The only real risk I see here is if customers are manually calling transport.send and have come to expect that it never throws.

So this could be considered behaviorally breaking, for custom transport implementations based on the base transport, right?
Any chance that we can still return the response and not throw? Or do you need it to throw for your transport?

[timfish]

So this could be considered behaviourally breaking, for custom transport implementations based on the base transport, right?

No, this does not change the behaviour, or expected behaviour of any existing transports or custom transports. A custom TransportRequestExecutor passed into createTransport has always been able to throw or not but errors were logged and ignored by createTransport.

Re-throwing the error here will only impact those that call the send function on Transport. So from a public API perspective, this would only result in behavioural changes for anyone calling client.getTransport().send(envelope).

[lforst]

Do you have a plan on doing something when the error is thrown? Otherwise I would just leave it be for now.

[timfish]

Do you have a plan on doing something when the error is thrown?

Yes, since an exception is the main failure mode for most of the TransportRequestExecutor implementations when the DSN cannot be resolved, (ie. while offline) this would be used to signal that the envelope should be queued and retried later.

We don't actually care about the exception type or any of the details though so I could alternatively include a flag in TransportMakeRequestResponse:

export type TransportMakeRequestResponse = {
  statusCode?: number;
  headers?: {
    [key: string]: string | null;
    'x-sentry-rate-limits': string | null;
    'retry-after': string | null;
  };
  failed?: true
};

So in createTransport, any caught errors would be converted to { failed: true }.

The main issue with this is that it might be confusing to those who are implementing transports. Should your custom transport throw or return { failed: true }? If a TransportRequestExecutor returns { failed: true }, should we treat it like an exception?

[lforst]

Do you have a plan on doing something when the error is thrown?

Yes, since an exception is the main failure mode for most of the TransportRequestExecutor implementations when the DSN cannot be resolved, (ie. while offline) this would be used to signal that the envelope should be queued and retried later.

We don't actually care about the exception type or any of the details though so I could alternatively include a flag in TransportMakeRequestResponse:

export type TransportMakeRequestResponse = {
  statusCode?: number;
  headers?: {
    [key: string]: string | null;
    'x-sentry-rate-limits': string | null;
    'retry-after': string | null;
  };
  failed?: true
};

So in createTransport, any caught errors would be converted to { failed: true }.

The main issue with this is that it might be confusing to those who are implementing transports. Should your custom transport throw or return { failed: true }? If a TransportRequestExecutor returns { failed: true }, should we treat it like an exception?

Ok thought about this way longer than I probably should. I think throwing is fine and it might have even been an oversight not to throw there in the first place. The only thing I would do is remove the log message on line 89 to only log in the place where we're actually handling the error.

The same sentiment goes towards returning the result. We should probably have made send return PromiseLike<TransportMakeRequestResponse> in the first place.

I say let's go through with it!

[timfish]

I would do is remove the log message on line 89

Oh yes, this will result in logging the error twice!

We should probably have made send return PromiseLike<TransportMakeRequestResponse> in the first place

I'll add a TODO (v8) comment to remove void in the next major.