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

Refactor: move DebugImage and DebugMeta to Sentry.Protocol #2815

Merged
merged 2 commits into from Nov 13, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Nov 9, 2023

Reasoning: some of the protocol classes (those sent to Sentry, mostly POCO) are residing directly in the Sentry namespace. Maybe we should move them to Protocol, although, it's a case-by-case decision because some may be used too frequently by SDK users so the migration wouldn't be worth any small benefit from the cleanup done here.

Let me know if you think any of these should move, or if I should abandon this completely. The list is a simple search of : IJsonSerializable

C:\dev\dotnet\src\Sentry\Breadcrumb.cs
  11,32: public sealed class Breadcrumb : IJsonSerializable

C:\dev\dotnet\src\Sentry\Package.cs
  9,29: public sealed class Package : IJsonSerializable

C:\dev\dotnet\src\Sentry\PersistedSessionUpdate.cs
  6,39: internal class PersistedSessionUpdate : IJsonSerializable

C:\dev\dotnet\src\Sentry\Request.cs
  28,29: public sealed class Request : IJsonSerializable

C:\dev\dotnet\src\Sentry\SdkVersion.cs
  11,32: public sealed class SdkVersion : IJsonSerializable

C:\dev\dotnet\src\Sentry\SentryMessage.cs
  19,35: public sealed class SentryMessage : IJsonSerializable

C:\dev\dotnet\src\Sentry\SentryStackFrame.cs
  10,38: public sealed class SentryStackFrame : IJsonSerializable

C:\dev\dotnet\src\Sentry\SentryStackTrace.cs
  43,31: public class SentryStackTrace : IJsonSerializable

C:\dev\dotnet\src\Sentry\SentryThread.cs
  10,34: public sealed class SentryThread : IJsonSerializable

C:\dev\dotnet\src\Sentry\SentryValues.cs
  9,39: internal sealed class SentryValues<T> : IJsonSerializable

C:\dev\dotnet\src\Sentry\User.cs
  10,26: public sealed class User : IJsonSerializable

C:\dev\dotnet\src\Sentry\UserFeedback.cs
  9,34: public sealed class UserFeedback : IJsonSerializable

C:\dev\dotnet\src\Sentry\ViewHierarchy.cs
  9,39:     public sealed class ViewHierarchy : IJsonSerializable

C:\dev\dotnet\src\Sentry\ViewHierarchyNode.cs
  8,41: public abstract class ViewHierarchyNode : IJsonSerializable

Note

There are also these types that implement IJsonSerializable in addition to other interfaces:

public sealed class Contexts : IDictionary<string, object>, IJsonSerializable
public sealed class SentryEvent : IEventLike, IJsonSerializable
public readonly struct SentryId : IEquatable<SentryId>, IJsonSerializable
public sealed class SentryStackFrame : IJsonSerializable
public class SessionUpdate : ISession, IJsonSerializable
public class Span : ISpanData, IJsonSerializable
public readonly struct SpanId : IEquatable<SpanId>, IJsonSerializable
public class Transaction : ITransactionData, IJsonSerializable

@jamescrosswell
Copy link
Collaborator

Maybe a naive question, but what's the purpose/benefit of the Protocol namespace? Is it just to help organise/identify the all the classes that get sent across the wire to Sentry? If so then yeah, makes sense to move all of the above... hopefully it's just global usings changes for SDK users, the most part.

@bruno-garcia
Copy link
Member

I'd prefer we optmize for the end user DX. Having to import another namespace just to send an event for example is not ideal, what's why some stuff was put in the root namespace, so import Sentry by itself is all you need.

@bruno-garcia
Copy link
Member

That said DebugMeta and stuff isn't really used a lot so it's fine to move, but we need to document a "before/after" so folks know what to when it breaks after bumping the SDK

@vaind vaind changed the title Refactor: move some classes to Protocol Refactor: move DebugImage and DebugMeta to Sentry.Protocol Nov 13, 2023
@vaind vaind marked this pull request as ready for review November 13, 2023 12:24
@@ -79,6 +79,7 @@ Additionally, we're dropping support for some of the old target frameworks, plea
- `DebugImage.ImageAddress` changed to `long?`. ([#2725](https://github.com/getsentry/sentry-dotnet/pull/2725))
- Contexts now inherits from `IDictionary` rather than `ConcurrentDictionary`. The specific dictionary being used is an implementation detail. ([#2729](https://github.com/getsentry/sentry-dotnet/pull/2729))
- Transaction names for ASP.NET Core are now consistently named `HTTP-VERB /path` (e.g. `GET /home`). Previously the leading forward slash was missing for some endpoints. ([#2808](https://github.com/getsentry/sentry-dotnet/pull/2808))
- `DebugImage` and `DebugMeta` moved to `Sentry.Protocol` namespace. ([#2815](https://github.com/getsentry/sentry-dotnet/pull/2815))
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 🚫 The changelog entry seems to be part of an already released section ## 4.0.0.
    Consider moving the entry to the ## Unreleased section, please.

@bitsandfoxes bitsandfoxes merged commit f661d07 into feat/4.0.0 Nov 13, 2023
15 of 19 checks passed
@bitsandfoxes bitsandfoxes deleted the refactor/protocol-classes branch November 13, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants