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

Remove 'Platform' from scope #1590

Closed
Tracked by #1585
bitsandfoxes opened this issue Apr 25, 2022 · 4 comments
Closed
Tracked by #1585

Remove 'Platform' from scope #1590

bitsandfoxes opened this issue Apr 25, 2022 · 4 comments
Assignees
Labels
Feature New feature or request
Milestone

Comments

@bitsandfoxes
Copy link
Contributor

Problem Statement

SentryEvents get the Platform property already set in the internal Constructor

Platform = Constants.Platform;

and the scope does not override it when it gets applied
other.Platform ??= Platform;

Does this mean setting the Platform when configuring the scope doesn't do anything?

Solution Brainstorm

We might as well remove the 'Platform' from the scope.

@bitsandfoxes bitsandfoxes added Feature New feature or request Platform: .NET labels Apr 25, 2022
@bruno-garcia bruno-garcia added this to the 4.0.0 milestone Apr 25, 2022
@bruno-garcia
Copy link
Member

platform should be always csharp (even if code was written in VB or F#) so makes sense to get rid of it

@mattjohnsonpint
Copy link
Contributor

platform should be always csharp

Yeah, probably it should have been called dotnet, but effectively it's the same for this purpose.

Though after reading the spec for stack traces, I believe we still need to let platform be set on a stack trace/frame so that we can override it if we bundle native SDKs in the future.

@mattjohnsonpint
Copy link
Contributor

We were wrong about this. The platform name won't always be "csharp". With the new Android support, if an event is generated from the embedded Android SDK, its platform will be "java". This would be evident on events exposed in a BeforeSend delegate. We shouldn't obsolete the property or remove it, as that will erroneously change the platform from "java" to "csharp" when roundtripping.

I'll revert the change.

@bruno-garcia
Copy link
Member

The protocol should allow different values to match the use case Matt described. But I believe there's still no need for Platform on the Scope though. So we could remove it from the scope.

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

When branches are created from issues, their pull requests are automatically linked.

4 participants