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

Expose ITransport and Friends Publicly #1018

Closed
Silvenga opened this issue May 28, 2021 · 6 comments
Closed

Expose ITransport and Friends Publicly #1018

Silvenga opened this issue May 28, 2021 · 6 comments
Labels
Feature New feature or request

Comments

@Silvenga
Copy link

We are trying to pump existing exceptions and error reports into Sentry from a central ingress point (the producers unfortunately cannot use the Sentry/Raven client directly as we need to control the transport and data captured). We were hoping to use Sentry as a SDK (as the REST documentation is a little hard to find).

The models exist and the serializiation logic is there, but everything e.g. EnvelopeHttpContent, HttpTransport, etc. that can use these models, are internal.

I propose decoupling SDK logic from Sentry proper or make these internal types public with no API SLA.

This would be on top of the work in #944

@bruno-garcia bruno-garcia added the Feature New feature or request label May 31, 2021
@bruno-garcia
Copy link
Member

Having a public/swappable transport is part of the SDK general design at Sentry and we had this internal for a while but we need to address this.

It requires some changes in the serialization bits I believe. @Tyrrrz knows more.

@Tyrrrz
Copy link
Contributor

Tyrrrz commented May 31, 2021

Related: #944

@Silvenga
Copy link
Author

Silvenga commented Jun 1, 2021

What needs to happen? I forked the project since I need a SDK right now.

  • I see tests failing when making the internal namespace public.
  • Changing visibility breaks the build:
    • Warns are created for missing xml documentation, which breaks from TreatWarningsAsErrors (e.g. the submodule).
    • Warns are created for using non CLS compliant types on public interfaces, which breaks from TreatWarningsAsErrors.

While we're at this, can we unseal classes? Unless Sentry is coupled to an implementation detail of these sealed classes - sealed just reduces flexibility on extenders.

(been neck deep in getting non-BCL exceptions to serialize across app domain boundaries from a LoadFrom context, sealed and internal types are currently my nemesis - FYI, not possible in net451+ without a lot of reflection into internal types because of a Framework bug + superfluous but forced serialization of unused types - why is serialization resolution logic internal....)

@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jun 1, 2021

Currently the mentioned PR (#944) seems to be abandoned by the original author. However it contains some useful discussions. If you want to go ahead and create a PR, we can help with specific issues/errors that you're having.

While we're at this, can we unseal classes? Unless Sentry is coupled to an implementation detail of these sealed classes - sealed just reduces flexibility on extenders.

Are there any particular sealed classes that you want to extend?

@Silvenga
Copy link
Author

Silvenga commented Jun 2, 2021

I think complete encapsulation of the SDK logic would be preferred (the models, the serialization logic, the transport abstractions, the http transport implementation) - but there currently exists some rather deep coupling e.g. exposing Exception on the sentry event, but requiring processing later on to copy the data into the final property for processing.

Exceptions in general would need to be decoupled away from the SDK - including "enhanced stacks". In my use-case, I don't have the original exception - I have to parse the stack trace to get it to place nice.

When just making the parts I needed public, I had a lot of difficulty resolving all the warnings and eventually just disabled errors on warnings. There were around 2k warnings on my branch (many came from the submodule for example).

My changes to encapsulate the SDK logic would be rather invasive to decouple the different components - then on top of that, there's the XML documentation warnings that would take a lot of effort to add - and the CLS compliant warnings. So getting anywhere without some 200 file PR, warnings would have to not break the build.

So this might not be a change I can do myself - rather the core maintainers of this SDK. FWIW, my fork does effectively work for our original needs and has entered production (custom transport pipeline and queuing) but it's likely not something that should be merged.

Are there any particular sealed classes that you want to extend?

Anything can be injected in to customize behavior, off the top of my head, app domain wrapping logic would be an example.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Apr 27, 2022

Completed with #1602 and #1560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
Archived in project
Development

No branches or pull requests

5 participants