Skip to content

Add configurable span filters.#220

Merged
zhming0 merged 2 commits intomainfrom
catkins/filter-trace-spans
Sep 17, 2024
Merged

Add configurable span filters.#220
zhming0 merged 2 commits intomainfrom
catkins/filter-trace-spans

Conversation

@catkins
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor 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!

Copy link
Contributor

@zhming0 zhming0 left a comment

Choose a reason for hiding this comment

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

I want this!! 🤩

@catkins
Copy link
Contributor Author

catkins commented Sep 17, 2024

@zhming0 fancy taking over the PR? I haven't had my head in the TA space just at the moment, and it could be good for you to have a play with doing a collector release

@zhming0 zhming0 merged commit c9b413b into main Sep 17, 2024
@zhming0 zhming0 deleted the catkins/filter-trace-spans branch September 17, 2024 00:34
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.

3 participants