-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Conversation
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. |
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 |
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 |
904f26d
to
31bf9bb
Compare
@@ -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)) |
There was a problem hiding this comment.
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.
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
Note
There are also these types that implement IJsonSerializable in addition to other interfaces: