Add Forge runtime transport

[1999]

This is a follow-up to #3655. This PR has the real Forge runtime transport implementation. If this PR gets merged, my next PR will be a change to the documentation. It will contain detailed information on how to use this transport in Forge runtime.

cc @nvenegas

[1999]

Meh, sorry for this. Looks like we have a blocker on our side for this PR: https://ecosystem.atlassian.net/browse/FRGE-273
I will try to push it.

[1999]

Update on the blocking dependencies:

• @forge/storage transitive dependency won't have "engines" restriction starting from its next patch release which is scheduled for June 29th
• PR to @forge/api to export simple types instead of type-only imports is in review and I'm hoping to get it merged by June 29th as well.

[1999]

Happy days. Our changes in the beta branch work fine. On Tuesday next week, we will publish new stable versions of the packages and I will update this PR with these versions.

[1999]

@kamilogorek okay, it's finally happening :) My changes have been published today and this PR should be ready to review.

[1999]

Ah, I forgot to replace our company NPM mirror with a public one. Will do in a min.

[1999]

Alright, it should be fine now.

[1999]

Hey @kamilogorek do you need any clarifying comments from me to review this PR? My plan was also to add documentation (https://github.com/getsentry/sentry-docs) after this PR but I can raise a PR in the docs repo in parallel.

[kamilogorek]

Hey @1999, the PR looks good, however after discussing with the team, we decided to not merge it into the main mono-repo in it's current shape (monorepo shape, not your PR). Current SDK architecture makes it quite hard for 3rd party contributions due to everything being stored in the main packages (in opposition to something like a basic plugin system, or per-feature structure like we are planning to do in the future - see: https://github.com/getsentry/sentry-javascript/tree/v7-dev/packages ).
By merging this, we'd make every node user to download parts of the code related to Forge in their codebase, no matter they use it or not (unless they treeshake it, which is not common in node envs). It also extends the support landscape that we'd have to perform, that we simply don't have enough resources for.

Would it be possible that you create a standalone repo/npm package out of this PR instead? I'm more than happy to do necessary changes to the SDK if there are any that'd prevent you from doing that.

Once you do that, we will happily link your package in our documentation with all the necessary explanations and section about Atlassian Forge.

Thanks!

[1999]

By merging this, we'd make every node user to download parts of the code related to Forge in their codebase, no matter they use it or not (unless they treeshake it, which is not common in node envs).

I don't quite agree with this. The way how transport is built is by passing Forge fetch function as a parameter. Also, it is a devDependency, therefore Sentry Node users won't download Forge parts.

It also extends the support landscape that we'd have to perform, that we simply don't have enough resources for.

That's a 100% valid reason and I fully agree with you.

Would it be possible that you create a standalone repo/npm package out of this PR instead? I'm more than happy to do necessary changes to the SDK if there are any that'd prevent you from doing that.

I remember that I tried to extract this but there were some types that I needed to heavily rely on and these types were not exported. I will take a look again and comment in this PR on what I will need to export.

[1999]

@kamilogorek thanks for finding time to review this. I really appreciate this!

[1999]

Looks like I will need these types:

import {
  HTTPModule,
  HTTPModuleClientRequest,
  HTTPModuleRequestIncomingMessage,
  HTTPModuleRequestOptions
} from '@sentry/node/dist/transports/base/http-module';

and also BaseTransport to extend it.

import { BaseTransport } from '@sentry/node/dist/transports';

Do you think it's reasonable if we export these types and class from @sentry/node package?

[kamilogorek]

I think this should already work:

import { Transports } from '@sentry/node';

class ForgeRuntimeTransport extends Transports.BaseTransport {
  // ...
}

as for types, I'll see what we can do there.

[kamilogorek]

I just realized that all the types you mentioned are self-contained and have no cross-references to the rest of the SDK. Because of this, and the fact that it's only 24LoC, it's rather safe and trivial to vendor them.

import { IncomingHttpHeaders, RequestOptions as HTTPRequestOptions } from 'http';
import { RequestOptions as HTTPSRequestOptions } from 'https';
import { URL } from 'url';

export type HTTPModuleRequestOptions = HTTPRequestOptions | HTTPSRequestOptions | string | URL;

export interface HTTPModuleRequestIncomingMessage {
  headers: IncomingHttpHeaders;
  statusCode?: number;
  on(event: 'data' | 'end', listener: () => void): void;
  setEncoding(encoding: string): void;
}

export interface HTTPModuleClientRequest {
  end(chunk: string): void;
  on(event: 'error', listener: () => void): void;
}

export interface HTTPModule {
  request(options: HTTPModuleRequestOptions, callback?: (res: HTTPModuleRequestIncomingMessage) => void): HTTPModuleClientRequest;
}

[1999]

@kamilogorek this one is challenging. What part of Sentry Node.JS API is considered "public"? As an example, if you decide to change BaseTransport.module type from HTTPModule to something else, will this be published as a new major version? If not, this makes my external module very fragile as it can work with v6.9.0 but it can break when you publish v6.10.0.

[kamilogorek]

That's a very valid question. We are trying to keep all the public methods/classes and their appropriate configs backward compatible. Eg. a change that you introduced (module) was not a major release, because it will still work just fine with any custom transport created before that release. We'll try to make sure it stays like this indefinitely (or until we do a major bump).

[1999]

@kamilogorek

We'll try to make sure it stays like this indefinitely (or until we do a major bump).

Shouldn't exporting types be one of the ways to enforce this? Imagine any of the custom transports created outside Sentry monorepo. The only thing they can rely on is a BaseTransport because it is exported. But what about the types used in this module? (HTTPModule, HTTPModuleClientRequest, HTTPModuleRequestIncomingMessage, HTTPModuleRequestOptions). Should all custom transports just copy them from Sentry repo? Why not just export them and allow custom transports authors to use these types in their code?

[kamilogorek]

I do agree with you, however, after 3 years of this version of SDK being alive, it's the first use-case like this that we had. The reason for this I believe is that BaseTransport covers all the defaults, and even without exposing these types, you can still create your own transport when you don't need to implement module. I just don't want to overflow https://getsentry.github.io/sentry-javascript/modules/node.html with every possible export, as it can get messy. If we'd have more scenarios like this in the past, we'd definitely rethink this approach.

[1999]

@kamilogorek fair enough. Thanks for explaining, I will continue with just BaseTransport.

[1999]

@kamilogorek good news: we managed to finally publish this custom transport. Here's how it is used in a real Forge app: https://bitbucket.org/atlassian/confluence-recent-edits-overview/src/master/src/sentry.ts.

Do you think it's fine if I add a "Transports" section to Node.JS transport docs as you have for PHP?

[kamilogorek]

Do you think it's fine if I add a "Transports" section to Node.JS transport docs as you have for PHP?

Definitely! I'm out next week, but feel free to assign me (or a whole @getsentry/team-webplatform) for a review and I'll take a look once I'm back.

[1999]

@kamilogorek have a great week off!

Here's a PR: getsentry/sentry-docs#3962

[1999]

@kamilogorek heads up that with the newest release of Forge CLI it is no longer possible to use Sentry in Forge apps. The root cause is webpack@5 that no longer bundles some polyfills and Sentry SDK for Node relies on "domain". Sad but there's not much I can do so I will have to deprecate the transport package.

[AbhiPrasad]

Sad to hear, but we opened #4633. Hopefully we can revisit this sometime in the future!

[kamilogorek]

Thanks for the heads-up @1999, appreciate that