Skip to content

Commit

Permalink
feat(profiling): Pass thread id to flamegraph generation (#74846)
Browse files Browse the repository at this point in the history
To generate a more accurate flamegraph, we want samples from the active
thread.

Depends on getsentry/snuba#6138
  • Loading branch information
Zylphrex authored Jul 25, 2024
1 parent ae783f0 commit a705fd4
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 8 deletions.
27 changes: 22 additions & 5 deletions src/sentry/profiles/flamegraph.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ class ContinuousProfileCandidate(TypedDict):
project_id: int
profiler_id: str
chunk_id: str
thread_id: str
start: str
end: str

Expand All @@ -287,6 +288,7 @@ class ProfileCandidates(TypedDict):
class ProfilerMeta:
project_id: int
profiler_id: str
thread_id: str
start: float
end: float

Expand Down Expand Up @@ -375,7 +377,6 @@ def get_profile_candidates_from_functions(self) -> ProfileCandidates:
}

def get_profile_candidates_from_transactions(self) -> ProfileCandidates:
# TODO: continuous profiles support
max_profiles = options.get("profiling.flamegraph.profile-set.size")

builder = DiscoverQueryBuilder(
Expand All @@ -388,6 +389,7 @@ def get_profile_candidates_from_transactions(self) -> ProfileCandidates:
"precise.finish_ts",
"profile.id",
"profiler.id",
"thread.id",
],
query=self.query,
limit=max_profiles,
Expand All @@ -399,9 +401,18 @@ def get_profile_candidates_from_transactions(self) -> ProfileCandidates:
builder.add_conditions(
[
Or(
[
Condition(Column("profile_id"), Op.IS_NOT_NULL),
Condition(Column("profiler_id"), Op.IS_NOT_NULL),
conditions=[
Condition(builder.resolve_column("profile.id"), Op.IS_NOT_NULL),
And(
conditions=[
Condition(builder.resolve_column("profiler.id"), Op.IS_NOT_NULL),
Condition(
Function("has", [Column("contexts.key"), "trace.thread_id"]),
Op.EQ,
1,
),
],
),
],
),
],
Expand All @@ -419,11 +430,12 @@ def get_profile_candidates_from_transactions(self) -> ProfileCandidates:
ProfilerMeta(
project_id=row["project.id"],
profiler_id=row["profiler.id"],
thread_id=row["thread.id"],
start=row["precise.start_ts"],
end=row["precise.finish_ts"],
)
for row in results["data"]
if row["profiler.id"] is not None
if row["profiler.id"] is not None and row["thread.id"]
]
)

Expand Down Expand Up @@ -513,11 +525,16 @@ def get_chunks_for_profilers(
"project_id": profiler_meta.project_id,
"profiler_id": profiler_meta.profiler_id,
"chunk_id": row["chunk_id"],
"thread_id": profiler_meta.thread_id,
"start": str(int(max(start, profiler_meta.start) * 1.0e9)),
"end": str(int(min(end, profiler_meta.end) * 1.0e9)),
}
)

# TODO: There is the possibility that different transactions use the same
# profiler, chunk and thread ids. So make sure to merge overlapping candidates
# to avoid using the same sample multiple times.

return continuous_profile_candidates

def _query_chunks_for_profilers(self, query: Query) -> Mapping[str, Any]:
Expand Down
8 changes: 8 additions & 0 deletions src/sentry/snuba/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,14 @@ class Columns(Enum):
issue_platform_name=None, # TODO: This doesn't exist yet
alias="profiler.id",
)
THREAD_ID = Column(
group_name=None,
event_name=None,
transaction_name="contexts[trace.thread_id]",
discover_name="contexts[trace.thread_id]",
issue_platform_name=None,
alias="thread.id",
)

REPLAY_ID = Column(
group_name=None,
Expand Down
35 changes: 32 additions & 3 deletions tests/sentry/api/endpoints/test_organization_profiling_profiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.http import HttpResponse
from django.urls import reverse
from rest_framework.exceptions import ErrorDetail
from snuba_sdk import Column, Condition, Function, Op, Or
from snuba_sdk import And, Column, Condition, Function, Op, Or

from sentry.profiles.flamegraph import FlamegraphExecutor
from sentry.profiles.utils import proxy_profiling_service
Expand Down Expand Up @@ -138,6 +138,7 @@ def store_transaction(
transaction=None,
profile_id=None,
profiler_id=None,
thread_id=None,
project=None,
):
data = load_data("transaction", timestamp=self.ten_mins_ago)
Expand All @@ -151,6 +152,11 @@ def store_transaction(
if profiler_id is not None:
data.setdefault("contexts", {}).setdefault("profile", {})["profiler_id"] = profiler_id

if thread_id is not None:
data.setdefault("contexts", {}).setdefault("trace", {}).setdefault("data", {})[
"thread.id"
] = thread_id

self.store_event(data, project_id=project.id if project else self.project.id)

return data
Expand Down Expand Up @@ -281,7 +287,18 @@ def test_queries_profile_candidates_from_transactions(self):
Or(
conditions=[
Condition(Column("profile_id"), Op.IS_NOT_NULL),
Condition(Column("profiler_id"), Op.IS_NOT_NULL),
And(
conditions=[
Condition(Column("profiler_id"), Op.IS_NOT_NULL),
Condition(
Function(
"has", [Column("contexts.key"), "trace.thread_id"]
),
Op.EQ,
1,
),
],
),
],
)
in snql_request.query.where
Expand Down Expand Up @@ -379,9 +396,11 @@ def test_queries_profile_candidates_from_transactions_with_data(

# this transaction has continuous profile with a matching chunk (to be mocked below)
profiler_id = uuid4().hex
thread_id = "12345"
profiler_transaction = self.store_transaction(
transaction="foo",
profiler_id=profiler_id,
thread_id=thread_id,
project=self.project,
)
start_timestamp = datetime.fromtimestamp(profiler_transaction["start_timestamp"])
Expand Down Expand Up @@ -430,7 +449,16 @@ def test_queries_profile_candidates_from_transactions_with_data(
Or(
conditions=[
Condition(Column("profile_id"), Op.IS_NOT_NULL),
Condition(Column("profiler_id"), Op.IS_NOT_NULL),
And(
conditions=[
Condition(Column("profiler_id"), Op.IS_NOT_NULL),
Condition(
Function("has", [Column("contexts.key"), "trace.thread_id"]),
Op.EQ,
1,
),
],
),
],
)
in snql_request.query.where
Expand All @@ -452,6 +480,7 @@ def test_queries_profile_candidates_from_transactions_with_data(
"project_id": self.project.id,
"profiler_id": profiler_id,
"chunk_id": chunk["chunk_id"],
"thread_id": thread_id,
"start": str(int(profiler_transaction["start_timestamp"] * 1e9)),
"end": str(int(profiler_transaction["timestamp"] * 1e9)),
},
Expand Down

0 comments on commit a705fd4

Please sign in to comment.