Allow to serialize events

[pchinery]

We are trying to adapt Sentry for our product, but we have a slightly different workflow than the usual "Sentry way", as we do not publish events immediately, but we allow our users to review them and send them afterwards. As this can be from within a service, we have to serialize the events in some way and then deserialize them to present them to the user as well as to send them to Sentry.

Right now, the WriteTo method from IJsonSerializable is marked as "do not use / only for internal reasons" and this warning probably is well justified.

We are using it anyway right now, as this is the only easy way to serialize this completely. To make it even worse, we have to parse the Exception to SentryExceptions. Due to MainExceptionProcessor being internal (again probably for good reasons), we have copied the class to our project.

I'd like to explore if opening these paths a little bit is something you think is worth exploring, as we would like to stick to this workflow and think this could be of benefit for others as well.

If you think this is something you would bring into the library, we would be happy to create a PR for this.

[bruno-garcia]

Wouldn't it be a better hook for your to implement ITransport instead? That happens after these callbacks such as EventProcessor, so the actual data sent to the user will be there, complete.

In your custom transport you could serialize the envelope, to display, and if approved by the user, you can just get an envelope object back, from the serialized form and send it to the SentryHttpTransport.

[pchinery]

Right now, this is beyond my understanding of the Sentry library, but it sounds quite promising. I will play a bit with this today. Thank you very much for pointing me in this direction, I will follow up here later.

[pchinery]

After doing some research, this approach sounds very good, as it does not require to do any custom serialization and we would not have to use the Sentry lib in an unintended way. Right now, ITransport is internal though and the Transport option is internal as well. Would it be the way to go to make this public? Or do you prefer a factory instead, like with SentryStackTraceFactory?

[bruno-garcia]

Seems like it was made internal here: 2e3a433#diff-51893aa74e97f7920707e0e7eafac7c116d293e5bddbc3f2c229d608da562c35R10

This is designed to be public API so looks like an oversight on our end. @Tyrrrz thoughts?

[Tyrrrz]

@bruno-garcia I believe, we reached this conclusion when we were adding caching transport. The idea was that, since transport is created based on SentryOptions, the user should not be able to provide it. Otherwise SentryOptions.CacheDirectoryPath etc are not used.

We create the transport here:

sentry-dotnet/src/Sentry/Internal/SdkComposer.cs

Lines 18 to 79 in a9304a0

	private ITransport CreateTransport()
	{
	// Override for tests
	if (_options.Transport is not null)
	{
	return _options.Transport;
	}
	if (_options.SentryHttpClientFactory is { })
	{
	_options.DiagnosticLogger?.LogDebug(
	"Using ISentryHttpClientFactory set through options: {0}.",
	_options.SentryHttpClientFactory.GetType().Name
	);
	}
	var httpClientFactory = _options.SentryHttpClientFactory ?? new DefaultSentryHttpClientFactory();
	var httpClient = httpClientFactory.Create(_options);
	var httpTransport = new HttpTransport(_options, httpClient);
	// Non-caching transport
	if (string.IsNullOrWhiteSpace(_options.CacheDirectoryPath))
	{
	return httpTransport;
	}
	// Caching transport
	var cachingTransport = new CachingTransport(httpTransport, _options);
	// If configured, flush existing cache
	if (_options.InitCacheFlushTimeout > TimeSpan.Zero)
	{
	_options.DiagnosticLogger?.LogDebug(
	"Flushing existing cache during transport activation up to {0}.",
	_options.InitCacheFlushTimeout
	);
	// Use a timeout to avoid waiting for too long
	using var timeout = new CancellationTokenSource(_options.InitCacheFlushTimeout);
	try
	{
	cachingTransport.FlushAsync(timeout.Token).GetAwaiter().GetResult();
	}
	catch (OperationCanceledException)
	{
	_options.DiagnosticLogger?.LogError(
	"Flushing timed out."
	);
	}
	catch (Exception ex)
	{
	_options.DiagnosticLogger?.LogFatal(
	"Flushing failed.",
	ex
	);
	}
	}
	return cachingTransport;
	}

[Tyrrrz]

Now that I look at it, we could still make it public and just wrap the provided ITransport inside CachingTransport.

[bruno-garcia]

That sounds good.

Would be good to add to the xml doc of CacheDirectoryPath that "Setting a path will wrap the transport defined in Transport with a CachingTransport" for clarity.

@pchinery is this something you'd be willing to open a PR for? We'd appreciate it

[pchinery]

@bruno-garcia: Of course, I'd be happy to create a PR, based on the idea to always wrap the provided ITransport in a CachingTransport

[pchinery]

I have now created the pull request #944. Please let me know your thoughts if matches your idea or if you want anything changed.

I have not been 100% sure about the format for the changelog.md, but I tried my best to mimic the other entries.

[mattjohnsonpint]

@pchinery - Sorry about the long delay on this. Can you please see my comment here and also see #1602, which should do the same as what you had in #944. If there's anything else you need for this, I'd like to try to address it before the next release. Thanks.