Fix don't overwrite the span status of unfinished spans by markushi · Pull Request #2859 · getsentry/sentry-java

markushi · 2023-07-20T07:58:43Z

📜 Description

Span status should remain untouched when being force-finished, instead of setting DEADLINE_EXCEEDED.

💡 Motivation and Context

Fixes #2730

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

github-actions · 2023-07-20T08:16:53Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	329.35 ms	349.02 ms	19.67 ms
Size	1.72 MiB	2.26 MiB	547.99 KiB

Previous results on branch: fix/dont-set-deadline-exceeded-for-unfinished-spans

Startup times

Revision	Plain	With Sentry	Diff
`b0815e7`	286.90 ms	322.02 ms	35.12 ms
`03438e5`	282.12 ms	366.40 ms	84.28 ms

App size

Revision	Plain	With Sentry	Diff
`b0815e7`	1.72 MiB	2.29 MiB	575.25 KiB
`03438e5`	1.72 MiB	2.29 MiB	575.21 KiB

sentry/src/main/java/io/sentry/SentryTracer.java

codecov · 2023-07-21T05:26:48Z

Codecov Report

Patch coverage: 87.56% and project coverage change: +0.18% 🎉

Comparison is base (9020d1b) 81.26% compared to head (964e980) 81.45%.
Report is 46 commits behind head on feat/7.0.0.

Additional details and impacted files

@@               Coverage Diff                @@
##             feat/7.0.0    #2859      +/-   ##
================================================
+ Coverage         81.26%   81.45%   +0.18%     
- Complexity         4560     4660     +100     
================================================
  Files               350      354       +4     
  Lines             16866    17152     +286     
  Branches           2272     2320      +48     
================================================
+ Hits              13706    13971     +265     
- Misses             2219     2231      +12     
- Partials            941      950       +9

Files Changed	Coverage Δ
...racing/SentrySpanClientHttpRequestInterceptor.java	`0.00% <0.00%> (ø)`
...ebflux/SentryWebFilterWithThreadLocalAccessor.java	`0.00% <0.00%> (ø)`
...racing/SentrySpanClientHttpRequestInterceptor.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/Breadcrumb.java	`85.04% <ø> (ø)`
sentry/src/main/java/io/sentry/HostnameCache.java	`90.69% <ø> (ø)`
sentry/src/main/java/io/sentry/Hub.java	`75.17% <ø> (ø)`
.../src/main/java/io/sentry/JsonObjectSerializer.java	`84.21% <ø> (ø)`
...ry/src/main/java/io/sentry/ProfilingTraceData.java	`78.51% <ø> (ø)`
.../main/java/io/sentry/ProfilingTransactionData.java	`52.52% <ø> (ø)`
sentry/src/main/java/io/sentry/Scope.java	`96.72% <0.00%> (-0.36%)`	⬇️
... and 83 more

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

romtsn

LGTM, but only one concern - we're not executing this block for JPC stuff, do we still need to handle it (at least the start date), in case of parent transaction is aborted?

sentry-java/sentry/src/main/java/io/sentry/Span.java

Lines 193 to 222 in 695d3a3

    
           if (options.isTrimStart() || options.isTrimEnd()) { 
        
             @Nullable SentryDate minChildStart = null; 
        
             @Nullable SentryDate maxChildEnd = null; 
        
             // The root span should be trimmed based on all children, but the other spans, like the 
        
             // jetpack composition should be trimmed based on its direct children only 
        
             final @NotNull List<Span> children = 
        
                 transaction.getRoot().getSpanId().equals(getSpanId()) 
        
                     ? transaction.getChildren() 
        
                     : getDirectChildren(); 
        
             for (final Span child : children) { 
        
               if (minChildStart == null || child.getStartDate().isBefore(minChildStart)) { 
        
                 minChildStart = child.getStartDate(); 
        
               } 
        
               if (maxChildEnd == null 
        
                   || (child.getFinishDate() != null && child.getFinishDate().isAfter(maxChildEnd))) { 
        
                 maxChildEnd = child.getFinishDate(); 
        
               } 
        
             } 
        
             if (options.isTrimStart() 
        
                 && minChildStart != null 
        
                 && startTimestamp.isBefore(minChildStart)) { 
        
               updateStartDate(minChildStart); 
        
             } 
        
             if (options.isTrimEnd() 
        
                 && maxChildEnd != null 
        
                 && (this.timestamp == null || this.timestamp.isAfter(maxChildEnd))) { 
        
               updateEndDate(maxChildEnd); 
        
             } 
        
           }

CHANGELOG.md

…inished-spans

markushi · 2023-08-18T08:32:22Z

LGTM, but only one concern - we're not executing this block for JPC stuff, do we still need to handle it (at least the start date), in case of parent transaction is aborted?

I looked into this in more detail now and I think it's fine this way as relay would anyway set the status to deadline_exceeded, rendering the meaning of start/end useless

stefanosiano

good!

Fix don't overwrite the span status of unfinished spans

9ca0246

markushi requested review from adinauer, romtsn and stefanosiano as code owners July 20, 2023 07:58

Update Changelog

5967fdb

stefanosiano approved these changes Jul 20, 2023

View reviewed changes

romtsn reviewed Jul 20, 2023

View reviewed changes

sentry/src/main/java/io/sentry/SentryTracer.java Outdated Show resolved Hide resolved

Don't set end time on spans

46cb8f9

romtsn approved these changes Jul 21, 2023

View reviewed changes

romtsn reviewed Jul 21, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

markushi added 3 commits July 21, 2023 10:44

Merge branch 'feat/7.0.0' into fix/dont-set-deadline-exceeded-for-unf…

d908b67

…inished-spans

Merge branch 'feat/7.0.0' into fix/dont-set-deadline-exceeded-for-unf…

dd1e080

…inished-spans

Merge branch 'feat/7.0.0' into fix/dont-set-deadline-exceeded-for-unf…

964e980

…inished-spans

stefanosiano approved these changes Aug 18, 2023

View reviewed changes

markushi merged commit bdf1379 into feat/7.0.0 Aug 28, 2023

markushi deleted the fix/dont-set-deadline-exceeded-for-unfinished-spans branch August 28, 2023 11:22

bruno-garcia mentioned this pull request Dec 14, 2023

Include unfinished spans in transactions getsentry/sentry-dotnet#1182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Fix don't overwrite the span status of unfinished spans#2859

Fix don't overwrite the span status of unfinished spans#2859
markushi merged 6 commits intofeat/7.0.0from
fix/dont-set-deadline-exceeded-for-unfinished-spans

markushi commented Jul 20, 2023

Uh oh!

github-actions bot commented Jul 20, 2023 •

edited

Loading

Previous results on branch: fix/dont-set-deadline-exceeded-for-unfinished-spans

Startup times

App size

Uh oh!

Uh oh!

codecov bot commented Jul 21, 2023 •

edited

Loading

Uh oh!

romtsn left a comment

Uh oh!

Uh oh!

markushi commented Aug 18, 2023

Uh oh!

stefanosiano left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (options.isTrimStart() \|\| options.isTrimEnd()) {
	@Nullable SentryDate minChildStart = null;
	@Nullable SentryDate maxChildEnd = null;

	// The root span should be trimmed based on all children, but the other spans, like the
	// jetpack composition should be trimmed based on its direct children only
	final @NotNull List<Span> children =
	transaction.getRoot().getSpanId().equals(getSpanId())
	? transaction.getChildren()
	: getDirectChildren();
	for (final Span child : children) {
	if (minChildStart == null \|\| child.getStartDate().isBefore(minChildStart)) {
	minChildStart = child.getStartDate();
	}
	if (maxChildEnd == null
	\|\| (child.getFinishDate() != null && child.getFinishDate().isAfter(maxChildEnd))) {
	maxChildEnd = child.getFinishDate();
	}
	}
	if (options.isTrimStart()
	&& minChildStart != null
	&& startTimestamp.isBefore(minChildStart)) {
	updateStartDate(minChildStart);
	}
	if (options.isTrimEnd()
	&& maxChildEnd != null
	&& (this.timestamp == null \|\| this.timestamp.isAfter(maxChildEnd))) {
	updateEndDate(maxChildEnd);
	}
	}

Uh oh!

Comments

Conversation

markushi commented Jul 20, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: fix/dont-set-deadline-exceeded-for-unfinished-spans

Startup times

App size

Uh oh!

Uh oh!

codecov bot commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

romtsn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markushi commented Aug 18, 2023

Uh oh!

stefanosiano left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jul 20, 2023 •

edited

Loading

codecov bot commented Jul 21, 2023 •

edited

Loading