This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

feat: Sentry context #237

Merged

bruno-garcia merged 8 commits into develop from sentry-context

Apr 20, 2018

Member

bruno-garcia commented Apr 18, 2018

Payload creating has runtime detection
Context is built lazily on first event
Root context is closed per event to avoid sys calls required to build it
.NET Framework 4.7.1 added to support NET Standard 2.0 paths (RuntimeInformation)
Warnings over obsolete calls removed from test projects
Nancy targets net471 also
Json.NET indirect reference to Nancy

bruno-garcia added 5 commits

April 13, 2018 12:01


          feat(context data)

ece3744


          Payload creation runs runtime detection

1e98e4e


          feat: context data for all events

e6e2102

* Context is built lazily on first event
* Root context is closed per event to avoid sys calls required to build
it
* .NET Framework 4.7.1 added to support NET Standar 2.0 paths
(RuntimeInformation)


          feat: removed Version from Runtime (comes with Name)

f51624c


          build: Cake build can build, test, pack

f41e918

* Warnings over obsolete calls removed from test projects
* Nancy targets net471 also
* Json.NET indirect reference to Nancy

bruno-garcia changed the title ~~Sentry context~~ WIP: Sentry context


          fix: Context loaded on SentryEvent

cd2003b

bruno-garcia requested review from asbjornu and jan-auer

April 18, 2018 16:15

bruno-garcia changed the title ~~WIP: Sentry context~~ feat: Sentry context

jan-auer approved these changes

View reviewed changes

Member

jan-auer left a comment

Great work on this. Note that to be fully API compatible, contexts could theoretically be renamed, for example:

{
  "runtime": {
    "name": ".NET Framework"
  },
  "asp": {
    "type": "runtime",
    "name": "ASP.NET"
  }
}

asbjornu suggested changes

View reviewed changes

Contributor

asbjornu left a comment

Great additions. I only have a few minor comments.

src/app/SharpRaven/Data/Context/App.cs

+                                  app.Version = asm.Version?.ToString();
+                              }
+                              using (var proc = Process.GetCurrentProcess())

Contributor

asbjornu Apr 18, 2018

Is it wise to .Dispose() the current process?

Member Author

bruno-garcia Apr 20, 2018

Yes. This instance was created by me at this location and should be disposed when not needed.

src/app/SharpRaven/Data/Context/Device.cs

+                      /// Retrieves the architecture of this device.
+                      /// </summary>
+                      /// <returns>Arch or null if unknown</returns>
+                      internal static string GetArchitecture() =>

Contributor

asbjornu Apr 18, 2018

All these internal methods only seem to be used in a private scope, so can they be changed to private instead?

Member Author

bruno-garcia Apr 20, 2018

Yes, I made them internal for testing but I'm just covering them through testing the whole Device at the end. I'll make them private.

Member Author

bruno-garcia Apr 20, 2018

It is actually used by tests*

src/app/SharpRaven/Data/SentryEvent.cs Outdated

+                      /// <seealso href="https://docs.sentry.io/clientdev/interfaces/contexts/"/>
+                      public Contexts Contexts
+                      {
+                          get { return this.contexts; }

Contributor

asbjornu Apr 18, 2018

Can't you just add an internal set; to this property so we get rid of the field?

Member Author

bruno-garcia Apr 20, 2018

Will do

src/app/SharpRaven/Utilities/EntryAssemblyNameLocator.cs

+                          return entryMethod?.Module?.Assembly?.GetName();
+                      }
+                      internal static MethodBase GetApplicationEntryMethod()

Contributor

asbjornu Apr 18, 2018

👍

src/app/SharpRaven/Utilities/RuntimeInfoHelper.cs Outdated

+                              if (displayName != null)
+                              {
+                                  monoVersion = displayName.Invoke(null, null) as string;
+                                  if (monoVersion != null)

Contributor

asbjornu Apr 18, 2018

Can't all of these null checks be collapsed into a chained null propagation (?.) expression?

Member Author

bruno-garcia Apr 20, 2018 •

edited

Loading

I was afraid it'd be less readable but looks fine. Will change.

src/tests/SharpRaven.UnitTests/Data/Context/AppTests.cs Outdated

+                      {
+                          var sut = App.Create();
+                          Assert.True(sut.StartTime.HasValue);

Contributor

asbjornu Apr 18, 2018

I think Assert.That(app.StartTime.HasValue, Is.True) reads better.

src/tests/SharpRaven.UnitTests/Data/Context/AppTests.cs Outdated

+                          var sut = App.Create();
+                          Assert.True(sut.StartTime.HasValue);
+                          Assert.AreNotEqual(default(DateTimeOffset), sut.StartTime.Value);

Contributor

asbjornu Apr 18, 2018

I think Assert.That(app.StartTime.Value, Is.Not.EqualTo(default(DateTimeOffset)) reads better.

src/tests/SharpRaven.UnitTests/Data/Context/AppTests.cs Outdated

+                          Assert.True(sut.StartTime.HasValue);
+                          Assert.AreNotEqual(default(DateTimeOffset), sut.StartTime.Value);
+                          Assert.Greater(DateTimeOffset.UtcNow, sut.StartTime);

Contributor

asbjornu Apr 18, 2018

I think Assert.That(app.StartTime, Is.GreaterThan(DateTimeOffset.UtcNow)) reads better.

src/tests/SharpRaven.UnitTests/Data/Context/AppTests.cs Outdated

+                      {
+                          var sut = App.Create();
+                          Assert.NotNull(sut.Version);

Contributor

asbjornu Apr 18, 2018

I think Assert.That(app.Version, Is.Not.Null) reads better.

src/tests/SharpRaven.UnitTests/Data/Context/AppTests.cs

+                          Assert.That(actual, Is.EqualTo(@case.ExpectedSerializationOutput));
+                      }
+                      public static IEnumerable<object[]> TestCases()

Contributor

asbjornu Apr 18, 2018

👍


          review: changes requested by @asbjornu

c232da0

Member Author

bruno-garcia commented Apr 20, 2018

@asbjornu could you please check again?


          Merge branch 'develop' into sentry-context

a2bf658

Contributor

asbjornu commented Apr 20, 2018

@bruno-garcia: GetArchitecture and GetApplicationEntryMethod are still internal, but otherwise I think it looks good.

Member Author

bruno-garcia commented Apr 20, 2018

@asbjornu I commented on the review: It's to be accessed by the Unit testing library.

Member Author

bruno-garcia commented Apr 20, 2018

@asbjornu if that's ok, could you please approve it? Thanks!

asbjornu approved these changes

View reviewed changes

bruno-garcia merged commit 4130daa into develop

bruno-garcia deleted the sentry-context branch

April 20, 2018 12:25

This was referenced Apr 23, 2018

Send TimeZone information with Contexts data #236

Closed

feat(contexts): Normalize known os.name to render UI context icons` getsentry/sentry#8231

Merged

Report Runtime information in reliable way. #244

Closed

meta: Release 3.0.0 #246

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet