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

Add dedicated error sampling#420

Merged
gbbr merged 2 commits into
masterfrom
bmermet/error-sampling
May 9, 2018
Merged

Add dedicated error sampling#420
gbbr merged 2 commits into
masterfrom
bmermet/error-sampling

Conversation

@bmermet

@bmermet bmermet commented May 7, 2018

Copy link
Copy Markdown

This PR add a dedicated error sampler to the trace-agent. The goal of this is to keep more error traces (which are the most interesting ones).

With this PR the trace-agent will maintain two different score samplers that will each have their own maximum TPS. One for traces containing errors and one for other traces. That way the TPS of errors that the agent sends to the API will be independent from the TPS of traces containing no errors.

@bmermet
bmermet requested review from gbbr and palazzem May 7, 2018 16:37
@bmermet
bmermet force-pushed the bmermet/error-sampling branch from 9800336 to cf647fe Compare May 7, 2018 16:39

@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.

Really nice @bmermet !

Comment thread cmd/trace-agent/agent.go
info.UpdatePreSampler(*a.Receiver.preSampler.Stats())
}

func traceContainsError(trace model.Trace) bool {

@gbbr gbbr May 8, 2018

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.

There's a lot of functions which iterate over the entire trace (e.g. GetRoot, ChildrenMap, etc.). We should possibly only do this once when we receive the trace and compute various stats about the trace such as if it has errors, its root, the children map and other things we need.

This is just a remark, no action needed here. Possibly a worthy improvement to be done in another PR or during a potential refactor.

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.

Agreed, we should definitely think of consolidating all this logic to iterate only once on the trace. I'll keep that for another PR though.

Comment thread cmd/trace-agent/sampler.go Outdated
}
}

// NewErrorsSampler creates a new empty error sampler ready to be started

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.

Nit. Can we end this sentence with a period? Also, it would be helpful (at least to me) to know what an "errors sampler" is. Maybe this is the right place to write about that?

Comment thread info/info.go Outdated
}

// UpdatePrioritySamplerInfo updates internal stats about priority sampking
// UpdatePrioritySamplerInfo updates internal stats about priority sampling

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.

Please put a period at the end of this sentence, and the next one further down.

Comment thread sampler/coresampler.go
)

// EngineType represent the type of a sampler engine
type EngineType int

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.

I like this a lot 👍 Gives for a much cleaner, easier to understand and read API.

Comment thread sampler/scoresampler.go Outdated
return s
}

// NewErrorEngine returns an initialized Sampler

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.

I think we can do a bit better with this comment. 😄

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.

Indeed :)

@gbbr
gbbr merged commit a6ad93b into master May 9, 2018
@gbbr gbbr added this to the 6.2.0 milestone May 9, 2018
@palazzem
palazzem deleted the bmermet/error-sampling branch May 18, 2018 08:47
@palazzem palazzem modified the milestones: 6.2.0, 6.3.0 May 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants