Skip to content
This repository was archived by the owner on Aug 30, 2019. It is now read-only.

Ingest old traces and align their stats on oldestTs#493

Merged
gbbr merged 2 commits into
masterfrom
benjamin/concentrator-oldest-ts
Oct 16, 2018
Merged

Ingest old traces and align their stats on oldestTs#493
gbbr merged 2 commits into
masterfrom
benjamin/concentrator-oldest-ts

Conversation

@LotharSee

@LotharSee LotharSee commented Oct 15, 2018

Copy link
Copy Markdown

This PR changes the way we aggregate and flush stats in the concentrator.

What it does.

  • We no longer drop traces in agent.Process. The concentrator takes care of handling traces of any time itself.
  • The previous constant of "2 active time buckets" is moved to bufferLen. You can see it previously used in if model.Now()-2*a.conf.BucketInterval.Nanoseconds() and if ts > now-2*c.bsize [1]. It still defaults to 2.
  • bufferLen drives two things: it is the number of active time buckets for which we ingest statistics and the age of the time buckets we flush (aka. the delay before flushing a bucket).
  • During a flush, we move from bufferLen +1 active buckets (2 in the past + the current one) to bufferLen (the current one + the old one). That's the same behavior as before.
  • During a flush, we flush only the buckets before now-c.bufferLen*bsize, like before. It means a latency of 1 to 2 bsize aka. 10s to 20s.
  • In practice, as a flush is triggered every bsize, we only flush one bucket, the one at alignedNow-c.bufferLen*bsize.
  • To deal with old spans (aka. older than oldestTs, where now + bufferLen*c.bsize < oldestTs < now + (bufferLen-1)*c.bsize) we basically count the span as if its time was oldestTs. It means any old data ends up in the oldest bucket we kept.
  • At boot, the Agent is stricter, only allowing/aligning stats to the current time bucket. That allows it not to flush a bucket for a time window it would have flushed before an Agent restart.

agent collector time buckets

[1] bsize is the length of a time bucket, it's isn't publicly configurable so consider it as always 10 seconds (there is a TODO in the code to remove its configuration interface).

It practice, we can summarize it to two core changes:

  • We no longer create time buckets for spans arbitrarily far in the past (but with a recent root). That was leading to arbitrarily late stats buckets, wrongly processed in the pipeline / overriding what got flushed in the past. Instead, its stats are now counted in the oldestTs bucket.
  • We no longer drop stats for traces with a root late in the past. Consistently with the previous point, it is counted in the oldestTs bucket.

TODOS left:

  • Configurable bufferLen.

@LotharSee
LotharSee requested a review from gbbr October 15, 2018 15:04
@LotharSee LotharSee changed the title Ingest old traces and align them on oldestTs Ingest old traces and align their stats on oldestTs Oct 15, 2018
@gbbr gbbr added this to the 6.6.0 milestone Oct 16, 2018

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some questions and a made a small grammar correction, but looks good.

Comment thread cmd/trace-agent/concentrator.go Outdated
// bucket duration in nanoseconds
bsize int64
// Timestamp of the oldest time bucket for which we still allow data.
// Any ingested stats older than it gets added to that bucket.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this not cause incorrect stats? e.g. trace comes in 2 hours in the past but stats get added to the oldest bucket which is 10min ago.

// Any ingested stats older than it gets added to that bucket.
oldestTs int64
// bufferLen is the number of 10s stats bucket we keep in memory before flushing them.
// It means that we can compute stats only for the last `bufferLen * bsize` and that we

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can compute stats only for the last bufferLen * bsize

This seems to overlap with the definition of oldestTs. I'm a bit confused. Is bufferLen * bsize the same as oldestTs ?


// alignTs returns the provided timestamp truncated to the bucket size.
// It gives us the start time of the time bucket in which such timestamp falls.
func alignTs(ts int64, bsize int64) int64 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we make bsize a constant here? (since it's not going to be configurable anymore). We could then avoid to keep passing it around and simply have it set in this file.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants