Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configurable span filters. #220

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

catkins
Copy link

@catkins catkins commented May 14, 2024

When using the collector, sometimes there can many uninteresting spans in the trace from things like selenium or other framework noise. This adds a seam to allow users to have control of filtering out uninteresting trace spans.

Usage

ignore_selenium_requests = ->(span) do
  return true unless span[:section] == "http"

  !span.dig(:detail, :url) =~ /^http://selenium:4444//
end

Buildkite::TestCollector.configure(hook: :rspec)
Buildkite::TestCollector.span_filters << ignore_selenium_requests

This doesn't need to be a lambda, any #call-able will work, as long as it returns a truthy value from the callback when we want to retain the span.

Multiple span_filters can be provided, and as long they all return a truthy value, then the span will be retained, this allows composing multiple filters together.

This would be a public interface to the library, so I'm happy to workshop the naming/semantics.

When using the collector, sometimes there is a lot of tracing noise from
things like selenium or other framework noise. This adds a seam to allow
users to have control of filtering out uninteresting trace spans.
@catkins catkins requested a review from a team as a code owner May 14, 2024 10:19
@wooly
Copy link

wooly commented May 14, 2024

My gut says that the use of the word 'filter' in the option is throwing me since I'm associating it with Array#Filter, which I think has the opposite semantics of this: 'keep the stuff for which the block returns true'. I know that it's inverted internally but it just feels ever so slightly off.

What if the option was just span_filter and it filtered out everything that returned false?

@catkins
Copy link
Author

catkins commented May 14, 2024

Yeah that's what I started with too actually and I went back and forth a bit.

Perhaps I'll make a more interesting filter that rejects a few other types of span to see how that would look.

@catkins
Copy link
Author

catkins commented May 15, 2024

It would be great if there was a block form of Buildkite::TestCollector.configure similar to how rspec or vcr get setup

Buildkite::TestCollector.configure(hook: :rspec) do |config|
  config.batch_size = 200
  config.span_filters << ->(span) do
    # ... filtering things
  end
end

@wooly
Copy link

wooly commented May 15, 2024

Yeah that feels like a much more expressive API for sure.

@catkins catkins changed the title Add trace_ignore_span callback Add configurable span filters. May 15, 2024
@catkins
Copy link
Author

catkins commented May 15, 2024

I shuffled things around slightly to be composable, and re-expressed the minimum duration filtering @pda added in #215 to use the new construct.

I also updated the PR description to show the new syntax.

@wooly
Copy link

wooly commented May 15, 2024

Looks neat!

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.

None yet

2 participants