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

Ignore null value on CocoaScopeObserver.SetTag #3948

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

Cheesebaron
Copy link
Contributor

Added a null check to ignore null values, to have the same behavior as on Android. While SetTag doesn't allow null for the value argument, on Android it just throws a warning rather than throwing an ArgumentNullException deeper in the stack.

Fixes #3946

Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Thanks for PR @Cheesebaron !

See my comment about testing vs samples but otherwise looks good 👍🏻

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.56%. Comparing base (495e742) to head (2da2322).
Report is 468 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3948      +/-   ##
==========================================
+ Coverage   75.73%   76.56%   +0.83%     
==========================================
  Files         357      389      +32     
  Lines       13466    14261     +795     
  Branches     2671     2869     +198     
==========================================
+ Hits        10198    10919     +721     
- Misses       2593     2639      +46     
- Partials      675      703      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bruno-garcia
Copy link
Member

Error: Assert.Contains() Failure: Item not found in set
Set:       ["RemoveRange", "FreeHGlobal", "Finalize", "MoveNext", "Clear", ···]
Not found: "MethodToBeLoaded"

  Failed Sentry.Profiling.Tests.SamplingTransactionProfilerTests.EventPipeSession_ReceivesExpectedCLREvents [2 s]
  Error Message:
   Assert.Contains() Failure: Item not found in set
Set:       ["RemoveRange", "FreeHGlobal", "Finalize", "MoveNext", "Clear", ···]
Not found: "MethodToBeLoaded"
  Stack Trace:
     at Sentry.Profiling.Tests.SamplingTransactionProfilerTests.EventPipeSession_ReceivesExpectedCLREvents() in /Users/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs:line 209
--- End of stack trace from previous location ---
  Standard Output Messages:
 [Debug 00:00:01.50]: Sleeping... time remaining: 250 ms
 [Debug 00:00:01.56]: Profiling is being cut-of after 50 ms because the transaction takes longer than that.
 [Debug 00:00:01.59]: Sleeping... time remaining: 161 ms
 [Debug 00:00:01.72]: Sleeping... time remaining: 33 ms
 [Debug 00:00:01.72]: Profiler has been stopped and has received all the samples up to the end time.
 [Debug 00:00:01.81]: Sleeping... time remaining: 250 ms
 [Debug 00:00:01.94]: Sleeping... time remaining: 115 ms
 [Warning 00:00:02.18]: Error during sampler profiler session shutdown.
     Exception: System.AggregateException: One or more errors occurred. (A task was canceled.)
  ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
    at System.Threading.Tasks.Task.GetExceptions(Boolean includeTaskCanceledExceptions)
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
    at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
    at System.Threading.Tasks.Task.Wait()
    at Sentry.Profiling.SampleProfilerSession.Stop() in /_/src/Sentry.Profiling/SampleProfilerSession.cs:line 135
    at Sentry.Profiling.SampleProfilerSession.Dispose() in /_/src/Sentry.Profiling/SampleProfilerSession.cs:line 146
    at Sentry.Profiling.Tests.SamplingTransactionProfilerTests.EventPipeSession_ReceivesExpectedCLREvents() in /Users/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs:line 209
    at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecutionContextCallback(Object s)
    at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
    at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread threadPoolThread)
    at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext()
    at Xunit.Sdk.AsyncTestSyncContext.<>c__DisplayClass7_0.<Post>b__0() in /_/src/xunit.execution/Sdk/AsyncTestSyncContext.cs:line 58
    at Xunit.Sdk.XunitWorkerThread.<>c.<QueueUserWorkItem>b__5_0(Object _) in /_/src/common/XunitWorkerThread.cs:line 37
    at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
    at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
    at System.Threading.Thread.StartCallback()
 --- End of stack trace from previous location ---
 
    --- End of inner exception stack trace ---
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
    at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
    at System.Threading.Tasks.Task.Wait()
    at Sentry.Profiling.SampleProfilerSession.Stop() in /_/src/Sentry.Profiling/SampleProfilerSession.cs:line 135

Looks like the profiiling test is flakey

@bruno-garcia
Copy link
Member

@Cheesebaron could you please add a changelog entry?

Instructions on the diff:

image

@bruno-garcia
Copy link
Member

The linux build has a different profiling failure:

Passed!  - Failed:     0, Passed:     2, Skipped:     0, Total:     2, Duration: 12 s - Sentry.Profiling.Tests.dll (net9.0)
Test Run Aborted.
Test run for /home/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Tests/bin/Release/net48/Sentry.Tests.dll (.NETFramework,Version=v4.8)
A total of 1 test files matched the specified pattern.
  Skipped Sentry.Tests.Protocol.SentryTransactionTests.Finish_SentryRequestTransactionGetsIgnored [1 ms]
The active test run was aborted. Reason: Test host process crashed : Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at Microsoft.Diagnostics.Tracing.TraceEventDispatcher.Lookup(EVENT_RECORD*)
   at Microsoft.Diagnostics.Tracing.Etlx.TraceLog.OnAllEventPipeEventsRealTime(Microsoft.Diagnostics.Tracing.TraceEvent)
   at Microsoft.Diagnostics.Tracing.TraceEventDispatcher.DoDispatch(Microsoft.Diagnostics.Tracing.TraceEvent)
   at Microsoft.Diagnostics.Tracing.EventPipeEventSource.DispatchEventRecord(EVENT_RECORD*)
   at Microsoft.Diagnostics.Tracing.EventPipe.EventCache.SortAndDispatch(Int64)
   at Microsoft.Diagnostics.Tracing.EventPipe.EventCache.ProcessEventBlock(Byte[])
   at Microsoft.Diagnostics.Tracing.EventPipeBlock.FromStream(FastSerialization.Deserializer)
   at FastSerialization.Deserializer.ReadObjectDefinition(FastSerialization.Tags, FastSerialization.StreamLabel)
   at FastSerialization.Deserializer.ReadObject()
   at Microsoft.Diagnostics.Tracing.EventPipeEventSource.Process()
   at Microsoft.Diagnostics.Tracing.Etlx.TraceLogEventSource.Process()
   at System.Threading.Tasks.Task`1[[System.Boolean, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].InnerInvoke()
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(System.Threading.Tasks.Task ByRef, System.Threading.Thread)

[xUnit.net 00:00:16.75]     Sentry.Tests.ProfilerTests.Profiler_RunningUnderFullClient_SendsProfileData [SKIP]
  Skipped Sentry.Tests.ProfilerTests.Profiler_RunningUnderFullClient_SendsProfileData [1 ms]
Results File: /home/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Profiling.Tests/TestResults/testresults_Linux_net8.0_2025021009[290](https://github.com/getsentry/sentry-dotnet/actions/runs/13235365202/job/36945141030?pr=3948#step:14:291)7.trx

cc @vaind

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@vaind
Copy link
Collaborator

vaind commented Feb 11, 2025

The active test run was aborted. Reason: Test host process crashed : Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

that's also on main, I need to roll back recent changes and figure out what's wrong. Don't let this PR be blocked by that.

@jamescrosswell jamescrosswell merged commit 19b5a7e into getsentry:main Feb 11, 2025
22 checks passed
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.

SetTag no longer allows null value
4 participants