Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Features

- Allow to configure the `ITransport` to support custom serialization scenarios (#944) @pchinery
- Default environment to "debug" if running with debugger attached (#978)

### Fixes
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/Envelopes/Envelope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace Sentry.Protocol.Envelopes
/// <summary>
/// Envelope.
/// </summary>
internal sealed class Envelope : ISerializable, IDisposable
public sealed class Envelope : ISerializable, IDisposable
{
private const string EventIdKey = "event_id";

Expand Down
6 changes: 5 additions & 1 deletion src/Sentry/Envelopes/EnvelopeItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Sentry.Protocol.Envelopes
/// <summary>
/// Envelope item.
/// </summary>
internal sealed class EnvelopeItem : ISerializable, IDisposable
public sealed class EnvelopeItem : ISerializable, IDisposable
{
private const string TypeKey = "type";
private const string TypeValueEvent = "event";
Expand Down Expand Up @@ -56,6 +56,10 @@ public EnvelopeItem(IReadOnlyDictionary<string, object?> header, ISerializable p
var value => Convert.ToInt64(value) // can be int, long, or another numeric type
};

/// <summary>
/// Tries to get the FileNameKey
/// </summary>
/// <returns>The FileNameKey or null, if not set</returns>
public string? TryGetFileName() => Header.GetValueOrDefault(FileNameKey) as string;

private async Task<MemoryStream> BufferPayloadAsync(CancellationToken cancellationToken = default)
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/Envelopes/ISerializable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Sentry.Protocol.Envelopes
/// <summary>
/// Represents a serializable entity.
/// </summary>
internal interface ISerializable
public interface ISerializable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was another reason @Tyrrrz wanted to make things internal.
He had some plans to refactor this so types don't need to implement this in order to be serialized.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's most likely something that goes beyond of my understanding of the code vision of the library right now. I'm happy to help, but if there was an idea how to do this already, I might not think in the direction @Tyrrrz wanted to bring this, so I think it's better to wait for him.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a temp workaround to avoid exposing ISerializable, let's make:

  • EnvelopeItem.Payload internal (or even private? can we do that?)
  • EnvelopeItem.ctor internal (we won't expose a public constructor for envelope)

It's ugly but I think exposing ISerializable right now will constrain us too much. I don't like exposing Envelope either but there's no way around it if we want to expose ITransport.

{
/// <summary>
/// Serializes the object to a stream.
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/Extensibility/ITransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Sentry.Extensibility
/// <summary>
/// An abstraction to the transport of the event.
/// </summary>
internal interface ITransport
public interface ITransport
{
/// <summary>
/// Sends the <see cref="Envelope" /> to Sentry asynchronously.
Expand Down
28 changes: 18 additions & 10 deletions src/Sentry/Internal/SdkComposer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,8 @@ public SdkComposer(SentryOptions options)
if (options.Dsn is null) throw new ArgumentException("No DSN defined in the SentryOptions");
}

private ITransport CreateTransport()
private HttpTransport CreateHttpTransport()
{
// Override for tests
if (_options.Transport is not null)
{
return _options.Transport;
}

if (_options.SentryHttpClientFactory is { })
{
_options.DiagnosticLogger?.LogDebug(
Expand All @@ -34,16 +28,30 @@ private ITransport CreateTransport()
var httpClientFactory = _options.SentryHttpClientFactory ?? new DefaultSentryHttpClientFactory();
var httpClient = httpClientFactory.Create(_options);

var httpTransport = new HttpTransport(_options, httpClient);
return new HttpTransport(_options, httpClient);
}

private ITransport CreateTransport()
{
ITransport transport;

if (_options.Transport is not null)
{
transport = _options.Transport;
}
else
{
transport = CreateHttpTransport();
}
Comment on lines +36 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ITransport transport;
if (_options.Transport is not null)
{
transport = _options.Transport;
}
else
{
transport = CreateHttpTransport();
}
var transport = _options.Transport ?? CreateHttpTransport();


// Non-caching transport
if (string.IsNullOrWhiteSpace(_options.CacheDirectoryPath))
{
return httpTransport;
return transport;
}

// Caching transport
var cachingTransport = new CachingTransport(httpTransport, _options);
var cachingTransport = new CachingTransport(transport, _options);

// If configured, flush existing cache
if (_options.InitCacheFlushTimeout > TimeSpan.Zero)
Expand Down
9 changes: 7 additions & 2 deletions src/Sentry/SentryOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Sentry.Http;
using Sentry.Integrations;
using Sentry.Internal;
using Sentry.Internal.Http;
using static Sentry.Internal.Constants;
using static Sentry.Constants;
using Runtime = Sentry.PlatformAbstractions.Runtime;
Expand All @@ -21,8 +22,11 @@ public class SentryOptions
{
private Dictionary<string, string>? _defaultTags;

// Override for tests
internal ITransport? Transport { get; set; }
/// <summary>
/// The <see cref="ITransport"/> to use for sending the events. If null, the <see cref="HttpTransport"/> will be used.
/// The Transport will be wrapped with a <see cref="CachingTransport"/>, if a <see cref="CacheDirectoryPath"/> is set.
/// </summary>
public ITransport? Transport { get; set; }

internal ISentryStackTraceFactory? SentryStackTraceFactory { get; set; }

Expand Down Expand Up @@ -153,6 +157,7 @@ public class SentryOptions
/// </example>
/// <see href="https://develop.sentry.dev/sdk/features/#event-sampling"/>
private float? _sampleRate;

/// <summary>
/// The optional sample rate.
/// </summary>
Expand Down
27 changes: 27 additions & 0 deletions test/Sentry.Tests/SentrySdkTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using FluentAssertions;
using NSubstitute;
using Sentry.Extensibility;
using Sentry.Internal;
using Sentry.Internal.Http;
using Sentry.Protocol.Envelopes;
using Sentry.Testing;
Expand Down Expand Up @@ -270,6 +271,32 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed()
.Should().BeEmpty();
}

[Fact]
public async Task Init_WithCustomTransport_UsesCaching()
{
// Arrange
using var cacheDirectory = new TempDirectory();

using var _ = SentrySdk.Init(o =>
{
o.Dsn = ValidDsnWithoutSecret;
o.DiagnosticLogger = _logger;
o.CacheDirectoryPath = cacheDirectory.Path;
o.Transport = new FakeFailingTransport();
});

// Act

SentrySdk.CaptureMessage("some message");
await SentrySdk.FlushAsync(TimeSpan.FromSeconds(5));

// Assert
Directory
.EnumerateFiles(cacheDirectory.Path, "*", SearchOption.AllDirectories)
.ToArray()
.Should().NotBeEmpty();
}

[Fact]
public void Disposable_MultipleCalls_NoOp()
{
Expand Down