New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 775572 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Enable easier testing of recording traces

Project Member Reported by tdres...@chromium.org, Oct 17 2017

Issue description

I want to, with minimal boiler plate, be able to:
1: Unit test that a trace point is added correctly.
2: Test that a trace point is added correctly across threads.

An example of the boilerplate currently required:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/mouse_latency_browsertest.cc?q=mouse_latency&sq=package:chromium
 
Status: Available (was: Untriaged)
I think we definitely want to improve this! Any thoughts on what the testing API should look like?
This depends on whether or not we're going to have the notion of a "Trace Point Object" in Chrome.

Ideally, I think we create this for testing, whether or not it exists elsewhere.

We'd then have a synchronous method to enable tracing (taking a TraceConfig), and another method to disable tracing and get an array of TracePoint objects.

Each TracePoint object would have the set of properties which are common to all trace points, and then perhaps a base::Value of extra properties.

We may want to clean up the TraceConfig constructor a bit.
____________________

Crazy low priority ideas:

I can imagine getting fancy with custom matchers. For example, something like:

EXPECT_THAT(TraceMatches(trace_point, "Name::name", TracePoint::Start, "{json:example, thing:_}");
Or if we don't care about the name.
EXPECT_THAT(TraceMatches(trace_point, testing::_, TracePoint::Start, "{json:example, thing:_}");




Neat idea! This is longer term but ultimately I think we want to move away from JSON and have components emit structured proto messages. Testing those will be a lot easier too -- it should be possible to get to pretty close to what you wrote.
Cc: lalitm@chromium.org hjd@chromium.org
I just stumbled upon the need of having this while doing a review today:

https://chromium-review.googlesource.com/c/chromium/src/+/685854/13/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc

I am starting thinking something along the lines of:


class FilterPredicate {...}
class CategoryEq : public FilterPredicate { ...} 
class EventNameEq : public FilterPredicate { ...} 


struct TestTraceEvent {
  const char* category;
  const char* event_name;
  timestamp;
  something TBD for the args
}


class TraceEventEnumerable {
  using iterator = whatever<TestTraceEvent>;
  iterator begin()
  iterator end();
  size_t size();

  iterator Find(Predicate);

  // Allows to compose filtering, see below
  TraceEventEnumerable Filter(FilterPredicate);
}
/
class TracingTestHelper {
  void StartTracing(const std::string& categories);
  void StartTracing(const TraceConfig&);
  void StopTracing();

  // These below are valid only after StopTracing():
  TraceEventEnumerable GetTraceEvents() const; 
  std::vector<int> thread_ids();
  std::vector<int> pids();
}


And the way I imagine this being used in a test is something like:

TracingTestHelper th;
th.StartTracing("foo,bar");
th.StopTracing()
auto evt = th.GetTraceEvents()
  .Filter(CategoryEq("foo"))
  .Filter(EventNameEq("whatever"))
  .Find(TimeStampGt(...))
EXPECT_TRUE(evt.valid());
EXPECT_STREQ(evt->name, "foo");

actually +hjd for ideas, as he really know a lot about the various gtest helpers. there might be even a more gtest-friendly way to to all this.

Well we already have the trace event analyzer[1] which does something similar to that filter.

Personally I prefer an internal proto-based utility -- e.g., go/csjff -- which gives really readable error messages when things don't match. I hope we can reuse it somehow.

[1] https://cs.chromium.org/chromium/src/base/test/trace_event_analyzer.h?rcl=042c62afc88095c7d46b4283eedf52479f149b8a&l=38
Just to clarify: Find returns the first event matching the predicate?
Should TestTraceEvent have a phase field?

This looks pretty reasonable to me.

I'd also probably want a 
bool TestTraceEvent::Matches(FilterPredicate).

Then if I want to assert complicated things on a TestTraceEvent, I can just write a fancy FilterPredicate.

My previous example probably turns into:
EXPECT_TRUE(event.Matches(MyFilterPredicate(testing::_, TracePoint::Start, "{json:example, thing:_}"));

> Well we already have the trace event analyzer[1] 
I know and was planning to really base this ont op of that.
IMHO what is missing is all the enable, disable, flush, callback, runloop, json decode interface. See CL linked in #0 as an example (it uses TraceEventAnalyzer)

> Find returns the first event matching the predicate?
Yup

> Should TestTraceEvent have a phase field?
Oh yeah definitely (concretely I'd just expose everything that TraceEventAnalyzer has, maybe making that a bit more api friendly)


> Then if I want to assert complicated things on a TestTraceEvent, I can just write a fancy FilterPredicate.
Yeah that'd be the idea

> My previous example probably turns into:
> EXPECT_TRUE(event.Matches(MyFilterPredicate(testing::_, TracePoint::Start, "{json:example, thing:_}"));

>  which gives really readable error messages when things don't match. 
Yeah that is doable with gtest. hjd@ showed me once how to do a custom printer for mismatching objects (Can't find the link right now)
Project Member

Comment 8 by sheriffbot@chromium.org, Oct 22

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment