Merge "Trace Redaction - Make Timeline Range Inclusive" into main
diff --git a/src/trace_redaction/collect_timeline_events_unittest.cc b/src/trace_redaction/collect_timeline_events_unittest.cc
index fab14fb..9b8790b 100644
--- a/src/trace_redaction/collect_timeline_events_unittest.cc
+++ b/src/trace_redaction/collect_timeline_events_unittest.cc
@@ -16,12 +16,13 @@
*/
#include "src/trace_redaction/collect_timeline_events.h"
+#include "protos/perfetto/trace/ftrace/sched.gen.h"
#include "src/base/test/status_matchers.h"
#include "src/trace_redaction/trace_redaction_framework.h"
#include "protos/perfetto/trace/ftrace/ftrace_event.gen.h"
#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.gen.h"
-#include "protos/perfetto/trace/ftrace/sched.gen.h"
+#include "protos/perfetto/trace/ftrace/task.gen.h"
#include "protos/perfetto/trace/ps/process_tree.gen.h"
#include "protos/perfetto/trace/trace_packet.gen.h"
@@ -29,23 +30,13 @@
namespace {
-constexpr uint64_t kSystemPackage = 0;
-constexpr uint64_t kUnityUid = 10252;
+constexpr uint64_t kPackage = 0;
+constexpr int32_t kPid = 1093;
-constexpr int32_t kZygotePid = 1093;
-constexpr int32_t kUnityPid = 7105;
-constexpr int32_t kUnityTid = 7127;
-
-// TODO(vaage): Need a better name and documentation.
-constexpr int32_t kPidWithNoOpen = 32;
-
-constexpr uint64_t kProcessTreeTimestamp = 6702093635419927;
-
-// These two timestamps are used to separate the packet and event times. A
-// packet can have time X, but the time can have time Y. Time Y should be used
-// for the events.
-constexpr uint64_t kThreadFreePacketTimestamp = 6702094703928940;
-constexpr uint64_t kThreadFreeOffset = 100;
+constexpr uint64_t kFullStep = 1000;
+constexpr uint64_t kTimeA = 0;
+constexpr uint64_t kTimeB = kFullStep;
+constexpr uint64_t kTimeC = kFullStep * 2;
} // namespace
@@ -53,167 +44,138 @@
// contains trace elements that should create timeline events.
class CollectTimelineEventsTest : public testing::Test {
protected:
- void SetUp() {
- CollectTimelineEvents collector;
-
- ASSERT_OK(collector.Begin(&context_));
-
- // Minimum ProcessTree information.
- {
- auto timestamp = kProcessTreeTimestamp;
-
- protos::gen::TracePacket packet;
- packet.set_timestamp(timestamp);
-
- auto* process_tree = packet.mutable_process_tree();
-
- auto* zygote = process_tree->add_processes();
- zygote->set_pid(kZygotePid);
- zygote->set_ppid(1);
- zygote->set_uid(kSystemPackage);
-
- auto* unity = process_tree->add_processes();
- unity->set_pid(kUnityPid);
- unity->set_ppid(1093);
- unity->set_uid(kUnityUid);
-
- auto* thread = process_tree->add_threads();
- thread->set_tid(kUnityTid);
- thread->set_tgid(kUnityPid);
-
- process_tree->set_collection_end_timestamp(timestamp);
-
- auto buffer = packet.SerializeAsString();
-
- protos::pbzero::TracePacket::Decoder decoder(buffer);
- ASSERT_OK(collector.Collect(decoder, &context_));
- }
-
- // Minimum proc free informations.
- {
- auto timestamp = kThreadFreePacketTimestamp;
-
- protos::gen::TracePacket packet;
- packet.set_timestamp(timestamp);
-
- auto* ftrace_event = packet.mutable_ftrace_events()->add_event();
- ftrace_event->set_timestamp(timestamp + kThreadFreeOffset);
- ftrace_event->set_pid(10); // kernel thread - e.g. "rcuop/0"
-
- auto* process_free = ftrace_event->mutable_sched_process_free();
- process_free->set_pid(kUnityTid);
-
- auto buffer = packet.SerializeAsString();
-
- protos::pbzero::TracePacket::Decoder decoder(buffer);
- ASSERT_OK(collector.Collect(decoder, &context_));
- }
-
- // Free a pid that neve started.
- {
- auto timestamp = kThreadFreePacketTimestamp;
-
- protos::gen::TracePacket packet;
- packet.set_timestamp(timestamp);
-
- auto* ftrace_event = packet.mutable_ftrace_events()->add_event();
- ftrace_event->set_timestamp(timestamp + kThreadFreeOffset);
- ftrace_event->set_pid(10); // kernel thread - e.g. "rcuop/0"
-
- auto* process_free = ftrace_event->mutable_sched_process_free();
- process_free->set_pid(kPidWithNoOpen);
-
- auto buffer = packet.SerializeAsString();
-
- protos::pbzero::TracePacket::Decoder decoder(buffer);
- ASSERT_OK(collector.Collect(decoder, &context_));
- }
-
- ASSERT_OK(collector.End(&context_));
- }
+ void SetUp() { ASSERT_OK(collector_.Begin(&context_)); }
Context context_;
+ CollectTimelineEvents collector_;
};
-class CollectTimelineFindsOpenEventTest
- : public CollectTimelineEventsTest,
- public testing::WithParamInterface<int32_t> {};
+TEST_F(CollectTimelineEventsTest, OpenEventForProcessTreeProcess) {
+ {
+ protos::gen::TracePacket packet;
+ packet.set_timestamp(kTimeA);
-TEST_P(CollectTimelineFindsOpenEventTest, NoOpenEventBeforeProcessTree) {
- auto pid = GetParam();
+ auto* process_tree = packet.mutable_process_tree();
- auto event =
- context_.timeline->FindPreviousEvent(kProcessTreeTimestamp - 1, pid);
- ASSERT_EQ(event.type, ProcessThreadTimeline::Event::Type::kInvalid);
+ auto* process = process_tree->add_processes();
+ process->set_pid(kPid);
+ process->set_ppid(1);
+ process->set_uid(kPackage);
+
+ auto buffer = packet.SerializeAsString();
+
+ protos::pbzero::TracePacket::Decoder decoder(buffer);
+
+ ASSERT_OK(collector_.Collect(decoder, &context_));
+ }
+
+ ASSERT_OK(collector_.End(&context_));
+
+ const auto* event = context_.timeline->GetOpeningEvent(kTimeA, kPid);
+ ASSERT_TRUE(event);
+ ASSERT_TRUE(event->valid());
}
-TEST_P(CollectTimelineFindsOpenEventTest, OpenEventOnProcessTree) {
- auto pid = GetParam();
+TEST_F(CollectTimelineEventsTest, OpenEventForProcessTreeThread) {
+ {
+ protos::gen::TracePacket packet;
+ packet.set_timestamp(kTimeA);
- auto event = context_.timeline->FindPreviousEvent(kProcessTreeTimestamp, pid);
- ASSERT_EQ(event.type, ProcessThreadTimeline::Event::Type::kOpen);
- ASSERT_EQ(event.pid, pid);
+ auto* process_tree = packet.mutable_process_tree();
+
+ auto* process = process_tree->add_threads();
+ process->set_tid(kPid);
+ process->set_tgid(1);
+
+ auto buffer = packet.SerializeAsString();
+
+ protos::pbzero::TracePacket::Decoder decoder(buffer);
+
+ ASSERT_OK(collector_.Collect(decoder, &context_));
+ }
+
+ ASSERT_OK(collector_.End(&context_));
+
+ const auto* event = context_.timeline->GetOpeningEvent(kTimeA, kPid);
+ ASSERT_TRUE(event);
+ ASSERT_TRUE(event->valid());
}
-TEST_P(CollectTimelineFindsOpenEventTest, OpenEventAfterProcessTree) {
- auto pid = GetParam();
+TEST_F(CollectTimelineEventsTest, OpenEventForNewTask) {
+ {
+ protos::gen::TracePacket packet;
+ auto* event = packet.mutable_ftrace_events()->add_event();
+ event->set_timestamp(kTimeA);
- auto event = context_.timeline->FindPreviousEvent(kProcessTreeTimestamp, pid);
- ASSERT_EQ(event.type, ProcessThreadTimeline::Event::Type::kOpen);
- ASSERT_EQ(event.pid, pid);
+ auto* new_task = event->mutable_task_newtask();
+ new_task->set_clone_flags(0);
+ new_task->set_comm("");
+ new_task->set_oom_score_adj(0);
+ new_task->set_pid(kPid);
+
+ auto buffer = packet.SerializeAsString();
+
+ protos::pbzero::TracePacket::Decoder decoder(buffer);
+
+ ASSERT_OK(collector_.Collect(decoder, &context_));
+ }
+
+ ASSERT_OK(collector_.End(&context_));
+
+ const auto* open_event = context_.timeline->GetOpeningEvent(kTimeA, kPid);
+ ASSERT_TRUE(open_event);
+ ASSERT_TRUE(open_event->valid());
}
-INSTANTIATE_TEST_SUITE_P(
- SystemProcess,
- CollectTimelineFindsOpenEventTest,
- testing::Values(kZygotePid, // System-level process/thread
- kUnityPid, // Process
- kUnityTid // Child thread. kUnityPid is the parent.
- ));
+TEST_F(CollectTimelineEventsTest, ProcFreeEndsThread) {
+ {
+ protos::gen::TracePacket packet;
-class CollectTimelineFindsFreeEventTest : public CollectTimelineEventsTest {};
+ auto* event = packet.mutable_ftrace_events()->add_event();
+ event->set_timestamp(kTimeA);
-TEST_F(CollectTimelineFindsFreeEventTest, UsesFtraceEventTime) {
- auto pid = kUnityTid;
+ auto* new_task = event->mutable_task_newtask();
+ new_task->set_clone_flags(0);
+ new_task->set_comm("");
+ new_task->set_oom_score_adj(0);
+ new_task->set_pid(kPid);
- // While this will be a valid event (type != invalid), it won't be the close
- // event.
- auto incorrect =
- context_.timeline->FindPreviousEvent(kThreadFreePacketTimestamp, pid);
- ASSERT_EQ(incorrect.type, ProcessThreadTimeline::Event::Type::kOpen);
+ auto buffer = packet.SerializeAsString();
- auto correct = context_.timeline->FindPreviousEvent(
- kThreadFreePacketTimestamp + kThreadFreeOffset, pid);
- ASSERT_EQ(correct.type, ProcessThreadTimeline::Event::Type::kClose);
-}
+ protos::pbzero::TracePacket::Decoder decoder(buffer);
+ ASSERT_OK(collector_.Collect(decoder, &context_));
+ }
-TEST_F(CollectTimelineFindsFreeEventTest, NoCloseEventBeforeFree) {
- auto pid = kUnityTid;
+ {
+ protos::gen::TracePacket packet;
- auto event =
- context_.timeline->FindPreviousEvent(kThreadFreePacketTimestamp - 1, pid);
- ASSERT_EQ(event.type, ProcessThreadTimeline::Event::Type::kOpen);
- ASSERT_EQ(event.pid, pid);
-}
+ auto* event = packet.mutable_ftrace_events()->add_event();
+ event->set_timestamp(kTimeB);
-// Whether or not AddsCloseOnFree and AddsCloseAfterFree are the same close
-// event is an implementation detail.
-TEST_F(CollectTimelineFindsFreeEventTest, AddsCloseOnFree) {
- auto pid = kUnityTid;
+ auto* process_free = event->mutable_sched_process_free();
+ process_free->set_comm("");
+ process_free->set_pid(kPid);
+ process_free->set_prio(0);
- auto event = context_.timeline->FindPreviousEvent(
- kThreadFreePacketTimestamp + kThreadFreeOffset, pid);
- ASSERT_EQ(event.type, ProcessThreadTimeline::Event::Type::kClose);
- ASSERT_EQ(event.pid, pid);
-}
+ auto buffer = packet.SerializeAsString();
-TEST_F(CollectTimelineFindsFreeEventTest, AddsCloseAfterFree) {
- auto pid = kUnityTid;
+ protos::pbzero::TracePacket::Decoder decoder(buffer);
+ ASSERT_OK(collector_.Collect(decoder, &context_));
+ }
- auto event = context_.timeline->FindPreviousEvent(
- kThreadFreePacketTimestamp + kThreadFreeOffset + 1, pid);
- ASSERT_EQ(event.type, ProcessThreadTimeline::Event::Type::kClose);
- ASSERT_EQ(event.pid, pid);
+ ASSERT_OK(collector_.End(&context_));
+
+ const auto* start = context_.timeline->GetOpeningEvent(kTimeA, kPid);
+ ASSERT_TRUE(start);
+ ASSERT_TRUE(start->valid());
+
+ // The end event was correctly set so that the free event is inclusive.
+ const auto* end = context_.timeline->GetOpeningEvent(kTimeB, kPid);
+ ASSERT_TRUE(end);
+ ASSERT_TRUE(end->valid());
+
+ const auto* after = context_.timeline->GetOpeningEvent(kTimeC, kPid);
+ ASSERT_FALSE(after);
}
} // namespace perfetto::trace_redaction
diff --git a/src/trace_redaction/process_thread_timeline.cc b/src/trace_redaction/process_thread_timeline.cc
index 43d3637..a428073 100644
--- a/src/trace_redaction/process_thread_timeline.cc
+++ b/src/trace_redaction/process_thread_timeline.cc
@@ -45,95 +45,116 @@
mode_ = Mode::kRead;
}
+const ProcessThreadTimeline::Event* ProcessThreadTimeline::GetOpeningEvent(
+ uint64_t ts,
+ int32_t pid) const {
+ PERFETTO_DCHECK(mode_ == Mode::kRead);
+
+ auto prev_open = QueryLeftMax(ts, pid, Event::Type::kOpen);
+ auto prev_close = QueryLeftMax(ts, pid, Event::Type::kClose);
+
+ // If there is no open event before ts, it means pid never started.
+ if (!prev_open) {
+ return nullptr;
+ }
+
+ // There is a close event that is strictly between the open event and ts, then
+ // the pid is considered free.
+ //
+ // |---------| ^ : pid is free
+ // ^ |---------| ^ : pid is free
+ // ^---------| : pid is active
+ // |---------^ : pid is active
+ // |----^----| : pid is active
+
+ // Both open and close are less than or equal to ts (QueryLeftMax).
+ uint64_t close = prev_close ? prev_close->ts : 0;
+ uint64_t open = prev_open->ts;
+
+ return close > open && close < ts ? nullptr : prev_open;
+}
+
bool ProcessThreadTimeline::PidConnectsToUid(uint64_t ts,
int32_t pid,
uint64_t uid) const {
PERFETTO_DCHECK(mode_ == Mode::kRead);
- auto event = FindPreviousEvent(ts, pid);
+ const auto* prev_open = QueryLeftMax(ts, pid, Event::Type::kOpen);
+ const auto* prev_close = QueryLeftMax(ts, pid, Event::Type::kClose);
for (size_t d = 0; d < kMaxSearchDepth; ++d) {
- // The thread/process was freed. It won't exist until a new open event.
- if (event.type != Event::Type::kOpen) {
+ // If there's no previous open event, it means this pid was never created.
+ // This should not happen.
+ if (!prev_open) {
return false;
}
+ // This get tricky here. If done wrong, proc_free events will fail because
+ // they'll report as disconnected when they could be connected to the
+ // package. Inclusive bounds are used here. In context, if a task_newtask
+ // event happens at time T, the pid exists at time T. If a proc_free event
+ // happens at time T, the pid is "shutting down" at time T but still exists.
+ //
+ // B E : B = begin
+ // . . E = end
+ // . .
+ // |---------| ^ : pid is free
+ // ^ |---------| : pid is free
+ // ^---------| : pid is active
+ // |---------^ : pid is active
+ // |----^----| : pid is active
+
+ // By definition, both open and close are less than or equal to ts
+ // (QueryLeftMax), so problem space is reduces.
+ auto close = prev_close ? prev_close->ts : 0;
+ auto open = prev_open->ts;
+
+ if (close > open && close < ts) {
+ return false; // Close is sitting between open and ts.
+ }
+
// TODO(vaage): Normalize the uid values.
- if (event.uid == uid) {
+ if (prev_open->uid == uid) {
return true;
}
- // If there is no parent, there is no way to keep searching.
- if (event.ppid == Event::kUnknownPid) {
- return false;
+ if (prev_open->ppid == Event::kUnknownPid) {
+ return false; // If there is no parent, there is no way to keep
+ // searching.
}
- event = FindPreviousEvent(ts, event.ppid);
+ auto ppid = prev_open->ppid;
+
+ prev_open = QueryLeftMax(ts, ppid, Event::Type::kOpen);
+ prev_close = QueryLeftMax(ts, ppid, Event::Type::kClose);
}
return false;
}
-ProcessThreadTimeline::Event ProcessThreadTimeline::FindPreviousEvent(
+const ProcessThreadTimeline::Event* ProcessThreadTimeline::QueryLeftMax(
uint64_t ts,
- int32_t pid) const {
- PERFETTO_DCHECK(mode_ == Mode::kRead);
-
- Event fake = Event::Close(ts, pid);
+ int32_t pid,
+ Event::Type type) const {
+ auto fake = Event::Close(0, pid);
// Events are sorted by pid, creating islands of data. This search is to put
// the cursor at the start of pid's island. Each island will be small (a
// couple of items), so searching within the islands should be cheap.
- auto at = std::lower_bound(events_.begin(), events_.end(), fake, OrderByPid);
+ auto it = std::lower_bound(events_.begin(), events_.end(), fake, OrderByPid);
+ auto end = std::upper_bound(events_.begin(), events_.end(), fake, OrderByPid);
- // `pid` was not found in `events_`.
- if (at == events_.end()) {
- return {};
- }
+ const Event* best = nullptr;
- // "no best option".
- Event best = {};
+ for (; it != end; ++it) {
+ bool replace = false;
- // Run through all events (related to this pid) and find the last event that
- // comes before ts. If the events were in order by time, the search could be
- // more efficient, but the gains are margin because:
- //
- // 1. The number of edge cases go up.
- //
- // 2. The code is harder to read.
- //
- // 3. The performance gains are minimal or non-existant because of the small
- // number of events.
- for (; at != events_.end() && at->pid == pid; ++at) {
- // This event is after "now" and can safely be ignored.
- if (at->ts > ts) {
- continue;
+ if (it->type == type && it->ts <= ts) {
+ replace = !best || it->ts > best->ts;
}
- // `at` is know to be before now. So it is always safe to accept an event.
- //
- // All ts values are positive. However, ts_at and ts_best are both less than
- // ts (see early condition), meaning they can be considered negative values.
- //
- // at best ts
- // <---+-----------+-------------+---->
- // 31 64 93
- //
- // at best ts
- // <---+-----------+-------------+---->
- // -62 -29 0
- //
- // This means that the latest ts value under ts is the closest to ts.
-
- if (best.type == Event::Type::kInvalid || at->ts > best.ts) {
- best = *at;
- }
-
- // This handles the rare edge case where an open and close event occur at
- // the same time. The close event must get priority. This is done by
- // allowing close events to use ">=" where as other events can only use ">".
- if (at->type == Event::Type::kClose && at->ts == best.ts) {
- best = *at;
+ if (replace) {
+ best = &(*it);
}
}
diff --git a/src/trace_redaction/process_thread_timeline.h b/src/trace_redaction/process_thread_timeline.h
index fdb70ba..6352ebe 100644
--- a/src/trace_redaction/process_thread_timeline.h
+++ b/src/trace_redaction/process_thread_timeline.h
@@ -99,8 +99,14 @@
// Returns true if a process/thread is connected to a package.
bool PidConnectsToUid(uint64_t ts, int32_t pid, uint64_t uid) const;
- // Finds the pid's last event before ts.
- Event FindPreviousEvent(uint64_t ts, int32_t pid) const;
+ // Get the opening event for a pid. If pid ends at ts, an opening event will
+ // still be returned.
+ const Event* GetOpeningEvent(uint64_t ts, int32_t pid) const;
+
+ // SELECT MAX(ts), * FROM events WHERE pid=@pid AND type=@type AND ts<=@ts
+ const Event* QueryLeftMax(uint64_t ts,
+ int32_t pid,
+ ProcessThreadTimeline::Event::Type type) const;
private:
enum class Mode {
diff --git a/src/trace_redaction/process_thread_timeline_unittest.cc b/src/trace_redaction/process_thread_timeline_unittest.cc
index 4e86a8a..a563b58 100644
--- a/src/trace_redaction/process_thread_timeline_unittest.cc
+++ b/src/trace_redaction/process_thread_timeline_unittest.cc
@@ -23,21 +23,6 @@
namespace {
-class SliceTestParams {
- public:
- SliceTestParams(uint64_t ts, int32_t pid, uint64_t uid)
- : ts_(ts), pid_(pid), uid_(uid) {}
-
- uint64_t ts() const { return ts_; }
- int32_t pid() const { return pid_; }
- uint64_t uid() const { return uid_; }
-
- private:
- uint64_t ts_;
- int32_t pid_;
- uint64_t uid_;
-};
-
constexpr uint64_t kTimeA = 0;
constexpr uint64_t kTimeB = 10;
constexpr uint64_t kTimeC = 20;
@@ -103,72 +88,122 @@
ProcessThreadTimeline timeline_;
};
-TEST_F(ProcessThreadTimelineTest, NoEventBeforeFirstSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeA, kPidB);
- ASSERT_EQ(event, invalid_);
+TEST_F(ProcessThreadTimelineTest, BeforeSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeA, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_FALSE(prev_open);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeA, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_FALSE(prev_close);
}
-TEST_F(ProcessThreadTimelineTest, OpenEventAtStartOfFirstSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeB, kPidB);
- ASSERT_EQ(event, pid_b_events_[0]);
+TEST_F(ProcessThreadTimelineTest, StartOfSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeB, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_b_events_[0]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeB, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_FALSE(prev_close);
}
-TEST_F(ProcessThreadTimelineTest, OpenEventWithinFirstSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeC, kPidB);
- ASSERT_EQ(event, pid_b_events_[0]);
+TEST_F(ProcessThreadTimelineTest, DuringSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeC, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_b_events_[0]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeC, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_FALSE(prev_close);
}
-TEST_F(ProcessThreadTimelineTest, CloseEventAtEndOfFirstSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeD, kPidB);
- ASSERT_EQ(event, pid_b_events_[1]);
+TEST_F(ProcessThreadTimelineTest, EndOfSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeD, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_b_events_[0]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeD, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_TRUE(prev_close);
+ ASSERT_EQ(*prev_close, pid_b_events_[1]);
}
-TEST_F(ProcessThreadTimelineTest, CloseEventBetweenSpans) {
- auto event = timeline_.FindPreviousEvent(kTimeE, kPidB);
- ASSERT_EQ(event, pid_b_events_[1]);
+// Even through its after a span, the previous open and close events should be
+// openned.
+TEST_F(ProcessThreadTimelineTest, AfterSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeE, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_b_events_[0]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeE, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_TRUE(prev_close);
+ ASSERT_EQ(*prev_close, pid_b_events_[1]);
}
-TEST_F(ProcessThreadTimelineTest, OpenEventAtStartOfSecondSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeF, kPidB);
- ASSERT_EQ(event, pid_b_events_[2]);
+// When a pid is reused, the new open event (for the reused pid) should be
+// returned, but the close from the previous span should be returned.
+TEST_F(ProcessThreadTimelineTest, StartOfSecondSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeF, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_b_events_[2]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeF, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_TRUE(prev_close);
+ ASSERT_EQ(*prev_close, pid_b_events_[1]);
}
-TEST_F(ProcessThreadTimelineTest, OpenEventWithinSecondSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeG, kPidB);
- ASSERT_EQ(event, pid_b_events_[2]);
+// Now that there is a second close event, both open and close events should
+// come from the same span.
+TEST_F(ProcessThreadTimelineTest, CloseOfSecondSpan) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeH, kPidB, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_b_events_[2]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeH, kPidB, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_TRUE(prev_close);
+ ASSERT_EQ(*prev_close, pid_b_events_[3]);
}
-TEST_F(ProcessThreadTimelineTest, CloseEventAtEndOfSecondSpan) {
- auto event = timeline_.FindPreviousEvent(kTimeH, kPidB);
- ASSERT_EQ(event, pid_b_events_[3]);
+TEST_F(ProcessThreadTimelineTest, BeforeSpanWithZeroDuration) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeA, kPidD, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_FALSE(prev_open);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeA, kPidD, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_FALSE(prev_close);
}
-// Pid B is active. But Pid C is not active. At this point, Pid C should report
-// as invalid event though another pid is active.
-TEST_F(ProcessThreadTimelineTest, InvalidEventWhenAnotherSpanIsActive) {
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeB, kPidB), pid_b_events_[0]);
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeB, kPidC), invalid_);
+TEST_F(ProcessThreadTimelineTest, SpanWithZeroDuration) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeC, kPidD, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
+ ASSERT_EQ(*prev_open, pid_d_events_[0]);
+
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeC, kPidD, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_TRUE(prev_close);
+ ASSERT_EQ(*prev_close, pid_d_events_[1]);
}
-// When both pids are active, they should both report as active (using their
-// open events).
-TEST_F(ProcessThreadTimelineTest, ConcurrentSpansBothReportAsActive) {
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeC, kPidB), pid_b_events_[0]);
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeC, kPidC), pid_c_events_[0]);
-}
+TEST_F(ProcessThreadTimelineTest, AfterSpanWithZeroDuration) {
+ auto prev_open = timeline_.QueryLeftMax(
+ kTimeE, kPidD, ProcessThreadTimeline::Event::Type::kOpen);
+ ASSERT_TRUE(prev_open);
-// There are three test cases here:
-//
-// 1. Before open/close
-// 2. At open/close
-// 3. After open/close
-//
-// Normally these would be tree different test cases, but the naming gets
-// complicated, so it is easier to do it in one case.
-TEST_F(ProcessThreadTimelineTest, ZeroDuration) {
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeB, kPidD), invalid_);
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeC, kPidD), pid_d_events_[1]);
- ASSERT_EQ(timeline_.FindPreviousEvent(kTimeD, kPidD), pid_d_events_[1]);
+ auto prev_close = timeline_.QueryLeftMax(
+ kTimeE, kPidD, ProcessThreadTimeline::Event::Type::kClose);
+ ASSERT_TRUE(prev_close);
}
// |----- UID A -----| |----- UID C -----|
diff --git a/src/trace_redaction/redact_process_events_unittest.cc b/src/trace_redaction/redact_process_events_unittest.cc
index 7961017..ac7c624 100644
--- a/src/trace_redaction/redact_process_events_unittest.cc
+++ b/src/trace_redaction/redact_process_events_unittest.cc
@@ -331,11 +331,12 @@
ASSERT_TRUE(process_free.comm().empty());
}
-TEST_F(RedactProcessFree, DropsCommAtProcessFree) {
+TEST_F(RedactProcessFree, KeepsCommAtProcessFree) {
redact_.emplace_modifier<ClearComms>();
- // The new task is for Pid A. Pid A is part of Uid A. Keep Uid A; But, Pid A
- // ends at kTimeB (that's when the free event occurs). Drop comm.
+ // The new task is for Pid A. Pid A is part of Uid A. Keep Uid A; Process free
+ // marks the end of Pid A, but process free is inclusive and Pid A is only
+ // free after the event.
context_.package_uid = kUidA;
context_.timeline->Append(ProcessThreadTimeline::Event::Close(kTimeB, kPidA));
@@ -359,7 +360,7 @@
ASSERT_EQ(process_free.pid(), kPidA);
ASSERT_TRUE(process_free.has_comm());
- ASSERT_TRUE(process_free.comm().empty());
+ ASSERT_EQ(process_free.comm(), kCommA);
}
TEST_F(RedactProcessFree, KeepTaskInPackage) {
