Skip to content

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Apr 13, 2024

I'm only raising this manually for documentation:

Spring currently configures the Brave AsyncSpanHandler with default settings. These defaults included 3 thresholds to flush a backlog, whichever comes first.

The estimated size in bytes threshold (queuedMaxBytes=1pct memory) has been disabled by default and deprecated, due to appearing pinned when virtual threads are in use (-Djdk.tracePinnedThreads). This is effectively no impact due to the other two flush thresholds (queuedMaxSpans=10000,messageTimeout=1s), most importantly that in the default case (such as spring-boot not setting anything), you are much more likely to hit 1s timeout prior to 1pct of memory of 10k spans.

See https://github.com/openzipkin/zipkin-reporter-java/releases/tag/3.4.0 for more

Trivia time

OpenTelemetry's defaults are also in use in BatchSpanProcessorBuilder. There is no queuedMaxBytes analogue in their setup (in hindsight a good thing probably).

  • otel maxQueueSize=2048 vs zipkin-reporter queuedMaxSpans=10000
  • otel scheduleDelay=5s vs zipkin-reporter messageTimeout=1s

My unsolicited 2p is that the maxQueueSize=2048 while arbitrary is better than the queuedMaxSpans=10000 in zipkin-reporter.. I thought it was 10 year old from original brave, but that was 500 🤷. I raised this issue as 2048 is good, even if maybe smaller could be better.

scheduleDelay=5s might not be a great default though. I do remember changing zipkin-reporter to messageTimeout=1s, as beginning devs are impatient and wondered what was wrong prior to 5s :p

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 13, 2024
@wilkinsona wilkinsona added type: dependency-upgrade A dependency upgrade and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 15, 2024
@wilkinsona wilkinsona added this to the 3.3.x milestone Apr 15, 2024
@wilkinsona wilkinsona self-assigned this Apr 15, 2024
@wilkinsona wilkinsona modified the milestones: 3.3.x, 3.x Apr 15, 2024
@wilkinsona wilkinsona removed their assignment Apr 15, 2024
@wilkinsona wilkinsona added the status: on-hold We can't start working on this issue yet label Apr 15, 2024
@wilkinsona
Copy link
Member

This may have to wait for Boot 3.4 depending on whether the observability team want to upgrade to a new minor post RC1.

@codefromthecrypt
Copy link
Contributor Author

hah maybe I over-explained and doomed it ;)

@wilkinsona wilkinsona removed the status: on-hold We can't start working on this issue yet label Apr 16, 2024
@wilkinsona wilkinsona modified the milestones: 3.x, 3.3.x Apr 16, 2024
@wilkinsona
Copy link
Member

The observability team have merged the upgrades on their side. We're good to go here.

@wilkinsona wilkinsona modified the milestones: 3.3.x, 3.3.0-RC1 Apr 16, 2024
wilkinsona pushed a commit that referenced this pull request Apr 16, 2024
@codefromthecrypt codefromthecrypt deleted the zipkin-reporter-3.4.0 branch April 16, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: dependency-upgrade A dependency upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants