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

fix: incorrect order when getting the last active span. #1094

Merged
merged 5 commits into from Jun 30, 2021

Conversation

robersonliou
Copy link
Contributor

@robersonliou robersonliou commented Jun 30, 2021

Here is an active span order issue.

when I started multi active span in the same transaction, the span tree in sentry ui was not what I expected.
For example, it should be:

--- http.server
      |
      --- controller span
            |
            --- service span
                  |
                  --- repository span

but what I got was:

--- http.server
      |
      --- controller span
            |
            --- service span
            |
            --- repository span

Here is the actual screenshot:
image

I've traced the source code, and found this issue was cause by TransactionTrace.GetLastActiveSpan().
The TransactionTrace stored the ongoing span in the ConcurrentBag<ISpan>, it worked fine!
But the default order of ConcurrentBag is just like FILO, if I put several active spans sequentially, the result would like as following table:

SpanName IsFinished StartTime
repository span false 2021/6/30 00:00:02
service span false 2021/6/30 00:00:01
controller span false 2021/6/30 00:00:00

It could be solved by reordering the span.

# before
public ISpan? GetLastActiveSpan() => Spans.LastOrDefault(s => !s.IsFinished);

# after
public ISpan? GetLastActiveSpan() => Spans.OrderBy(s => s.StartTimestamp).LastOrDefault(s => !s.IsFinished);

@robersonliou robersonliou changed the title fix: incorrect order for getting the last active span. fix: incorrect order when getting the last active span. Jun 30, 2021
@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jun 30, 2021

Thank you!

Can you please add a changelog entry inside Changelog.md/Unreleased/Fixes section?

- Fixed incorrect order when getting the last active span ([#1094](https://github.com/getsentry/sentry-dotnet/pull/1094))

@Tyrrrz Tyrrrz added Bug Something isn't working Product: Performance labels Jun 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2021

Codecov Report

Merging #1094 (5a9cedf) into main (4c91cc6) will decrease coverage by 1.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1094      +/-   ##
==========================================
- Coverage   80.70%   79.06%   -1.64%     
==========================================
  Files         194      194              
  Lines        6359     6359              
  Branches     1411     1411              
==========================================
- Hits         5132     5028     -104     
- Misses        764      876     +112     
+ Partials      463      455       -8     
Impacted Files Coverage Δ
src/Sentry/TransactionTracer.cs 95.50% <100.00%> (ø)
src/Sentry/PlatformAbstractions/FrameworkInfo.cs 0.00% <0.00%> (-100.00%) ⬇️
...ntry/PlatformAbstractions/RegistryKeyExtensions.cs 0.00% <0.00%> (-100.00%) ⬇️
...Sentry/PlatformAbstractions/FrameworkInfo.NetFx.cs 0.00% <0.00%> (-71.24%) ⬇️
...rmAbstractions/NetFxInstallationsEventProcessor.cs 4.54% <0.00%> (-68.19%) ⬇️
...ntry/Integrations/NetFxInstallationsIntegration.cs 33.33% <0.00%> (-66.67%) ⬇️
...ntry/PlatformAbstractions/FrameworkInstallation.cs 25.00% <0.00%> (-37.50%) ⬇️
src/Sentry/PlatformAbstractions/RuntimeInfo.cs 53.44% <0.00%> (-5.18%) ⬇️
src/Sentry/Internal/MainExceptionProcessor.cs 88.52% <0.00%> (-4.92%) ⬇️
src/Sentry/Internal/AppDomainAdapter.cs 66.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c91cc6...5a9cedf. Read the comment docs.

@bruno-garcia
Copy link
Member

Thanks @robersonliou !

@bruno-garcia bruno-garcia enabled auto-merge (squash) June 30, 2021 17:07
@bruno-garcia bruno-garcia merged commit 0746c49 into getsentry:main Jun 30, 2021
@robersonliou
Copy link
Contributor Author

@bruno-garcia It seems nothing update in v3.6.1 assembly.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Product: Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants