Skip to content

Revise FlushTimer implementation, add isRoot - #48

Merged
ladd merged 3 commits into
mainfrom
lvt-revise-timer
Nov 13, 2025
Merged

Revise FlushTimer implementation, add isRoot#48
ladd merged 3 commits into
mainfrom
lvt-revise-timer

Conversation

@ladd

@ladd ladd commented Nov 13, 2025

Copy link
Copy Markdown

After some device testing, I revised the idle timer & root span approach a bit.

  • Don't create new root spans until they are needed
  • Suspend the timer on flushTrace -- it will be be resumed when new span is created.
  • Use DispatchTimerInterval more uniformly
  • Add isRoot to Span
  • swiftformat
  • Tests

@bachand
@jparise

@bachand bachand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice!

}
}

root.retireCallback = retire // initialization order

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does "retire" mean? is is the same as a span "ending" or more nuanced?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's just the process of handling ended spans: batching it for reporting is pretty much it.


/// Flushes the root span, and cycles the trace id
public func flushTrace() {
idleTimer?.suspend()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

let priorRoot = _root
traceId = Identifiers.generateTraceId()
_root = Span(name: "root", traceId: traceId, parentId: nil, retireCallback: retire)
_root = nil // will be recreated on next access

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍


var suspended = false

var _flushInterval: TimeInterval

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can/should this be private?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, done.

Comment thread Tests/NautilusTelemetryTests/Tracing/SpanTests.swift Outdated
}

class TestReporter : NautilusTelemetryReporter {
func testTracerRootSpanIsRoot() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment on lines +109 to +114
timer.flushInterval = 0.1

XCTAssertFalse(timer.suspended)

wait(for: [expectation2], timeout: 1.0)
XCTAssertEqual(handlerCallCount, 2)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

ladd and others added 2 commits November 13, 2025 10:17
Co-authored-by: Michael Bachand <bachand.michael@gmail.com>
@ladd
ladd merged commit dec6896 into main Nov 13, 2025
4 checks passed
@ladd
ladd deleted the lvt-revise-timer branch November 13, 2025 18:59
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.

2 participants