Skip to content

Commit

Permalink
fix(explore): Search type disagreements between snql and rpc (#83518)
Browse files Browse the repository at this point in the history 
Snql and RPC disagree on the search type of a few fields. Make them all
numbers. Will try to update to RPC so this doesn't happen again as that
will be the source of truth in the future.
  • Loading branch information
@Zylphrex
Zylphrex authored Jan 15, 2025
1 parent 01116d3 commit 950a9db
Show file tree
Hide file tree
Showing 3 changed files with 202 additions and 45 deletions.
10 changes: 7 additions & 3 deletions src/sentry/search/eap/columns.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,19 +354,23 @@ def datetime_processor(datetime_string: str) -> str:
),
# These fields are extracted from span measurements but were accessed
# 2 ways, with + without the measurements. prefix. So expose both for compatibility.
simple_measurements_field("cache.item_size", secondary_alias=True),
simple_measurements_field("cache.item_size", search_type="byte", secondary_alias=True),
ResolvedColumn(
public_alias="cache.item_size",
internal_name="cache.item_size",
search_type="byte",
),
simple_measurements_field("messaging.message.body.size", secondary_alias=True),
simple_measurements_field(
"messaging.message.body.size", search_type="byte", secondary_alias=True
),
ResolvedColumn(
public_alias="messaging.message.body.size",
internal_name="messaging.message.body.size",
search_type="byte",
),
simple_measurements_field("messaging.message.receive.latency", secondary_alias=True),
simple_measurements_field(
"messaging.message.receive.latency", search_type="millisecond", secondary_alias=True
),
ResolvedColumn(
public_alias="messaging.message.receive.latency",
internal_name="messaging.message.receive.latency",
Expand Down
5 changes: 5 additions & 0 deletions src/sentry/utils/snuba.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ def log_snuba_info(content):
# We should be able to delete origin.transaction and just use transaction
"origin.transaction": "segment_name",
# Copy paste, unsure if this is truth in production
"cache.item_size": "attr_num[cache.item_size]",
"messaging.message.body.size": "attr_num[messaging.message.body.size]",
"messaging.message.receive.latency": "attr_num[messaging.message.receive.latency]",
"messaging.message.retry.count": "attr_num[messaging.message.retry.count]",
"messaging.destination.name": "attr_str[sentry.messaging.destination.name]",
"messaging.message.id": "attr_str[sentry.messaging.message.id]",
"span.status_code": "attr_str[sentry.status_code]",
Expand Down Expand Up @@ -1935,6 +1939,7 @@ def is_duration_measurement(key):
"measurements.time_to_full_display",
"measurements.time_to_initial_display",
"measurements.inp",
"measurements.messaging.message.receive.latency",
]


Expand Down
232 changes: 190 additions & 42 deletions tests/snuba/api/endpoints/test_organization_events_span_indexed.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -629,48 +629,7 @@ def test_count_field_type(self):
assert response.data["meta"]["units"] == {"count()": None}
assert response.data["data"] == [{"count()": 0}]

def test_simple_measurements(self):
keys = [
("app_start_cold", "duration", "millisecond"),
("app_start_warm", "duration", "millisecond"),
("frames_frozen", "number", None), # should be integer but keeping it consistent
("frames_frozen_rate", "percentage", None),
("frames_slow", "number", None), # should be integer but keeping it consistent
("frames_slow_rate", "percentage", None),
("frames_total", "number", None), # should be integer but keeping it consistent
("time_to_initial_display", "duration", "millisecond"),
("time_to_full_display", "duration", "millisecond"),
("stall_count", "number", None), # should be integer but keeping it consistent
("stall_percentage", "percentage", None),
("stall_stall_longest_time", "number", None),
("stall_stall_total_time", "number", None),
("cls", "number", None),
("fcp", "duration", "millisecond"),
("fid", "duration", "millisecond"),
("fp", "duration", "millisecond"),
("inp", "duration", "millisecond"),
("lcp", "duration", "millisecond"),
("ttfb", "duration", "millisecond"),
("ttfb.requesttime", "duration", "millisecond"),
("score.cls", "number", None),
("score.fcp", "number", None),
("score.fid", "number", None),
("score.inp", "number", None),
("score.lcp", "number", None),
("score.ttfb", "number", None),
("score.total", "number", None),
("score.weight.cls", "number", None),
("score.weight.fcp", "number", None),
("score.weight.fid", "number", None),
("score.weight.inp", "number", None),
("score.weight.lcp", "number", None),
("score.weight.ttfb", "number", None),
("cache.item_size", "number", None),
("messaging.message.body.size", "number", None),
("messaging.message.receive.latency", "number", None),
("messaging.message.retry.count", "number", None),
]

def _test_simple_measurements(self, keys):
self.store_spans(
[
self.create_span(
Expand Down Expand Up @@ -722,6 +681,51 @@ def test_simple_measurements(self):
}
]

def test_simple_measurements(self):
keys = [
("app_start_cold", "duration", "millisecond"),
("app_start_warm", "duration", "millisecond"),
("frames_frozen", "number", None), # should be integer but keeping it consistent
("frames_frozen_rate", "percentage", None),
("frames_slow", "number", None), # should be integer but keeping it consistent
("frames_slow_rate", "percentage", None),
("frames_total", "number", None), # should be integer but keeping it consistent
("time_to_initial_display", "duration", "millisecond"),
("time_to_full_display", "duration", "millisecond"),
("stall_count", "number", None), # should be integer but keeping it consistent
("stall_percentage", "percentage", None),
("stall_stall_longest_time", "number", None),
("stall_stall_total_time", "number", None),
("cls", "number", None),
("fcp", "duration", "millisecond"),
("fid", "duration", "millisecond"),
("fp", "duration", "millisecond"),
("inp", "duration", "millisecond"),
("lcp", "duration", "millisecond"),
("ttfb", "duration", "millisecond"),
("ttfb.requesttime", "duration", "millisecond"),
("score.cls", "number", None),
("score.fcp", "number", None),
("score.fid", "number", None),
("score.inp", "number", None),
("score.lcp", "number", None),
("score.ttfb", "number", None),
("score.total", "number", None),
("score.weight.cls", "number", None),
("score.weight.fcp", "number", None),
("score.weight.fid", "number", None),
("score.weight.inp", "number", None),
("score.weight.lcp", "number", None),
("score.weight.ttfb", "number", None),
("messaging.message.receive.latency", "duration", "millisecond"),
("messaging.message.retry.count", "number", None),
# size fields aren't property support pre-RPC
("cache.item_size", "number", None),
("messaging.message.body.size", "number", None),
]

self._test_simple_measurements(keys)

def test_environment(self):
self.create_environment(self.project, name="prod")
self.create_environment(self.project, name="test")
Expand Down Expand Up @@ -1505,6 +1509,47 @@ def test_filtering_numeric_attr(self):
},
]

def test_byte_fields(self):
self.store_spans(
[
self.create_span(
{
"description": "foo",
"data": {
"cache.item_size": 1,
"messaging.message.body.size": 2,
},
},
start_ts=self.ten_mins_ago,
),
],
is_eap=self.is_eap,
)

response = self.do_request(
{
"field": [
"cache.item_size",
"measurements.cache.item_size",
"messaging.message.body.size",
"measurements.messaging.message.body.size",
],
"project": self.project.id,
"dataset": self.dataset,
}
)

assert response.data["data"] == [
{
"id": mock.ANY,
"project.name": self.project.slug,
"cache.item_size": 1.0,
"measurements.cache.item_size": 1.0,
"measurements.messaging.message.body.size": 2.0,
"messaging.message.body.size": 2.0,
},
]


class OrganizationEventsEAPRPCSpanEndpointTest(OrganizationEventsEAPSpanEndpointTest):
"""These tests aren't fully passing yet, currently inheriting xfail from the eap tests"""
Expand Down Expand Up @@ -1929,3 +1974,106 @@ def test_is_not_transaction(self):
},
]
assert meta["dataset"] == self.dataset

def test_byte_fields(self):
self.store_spans(
[
self.create_span(
{
"description": "foo",
"data": {
"cache.item_size": 1,
"messaging.message.body.size": 2,
},
},
start_ts=self.ten_mins_ago,
),
],
is_eap=self.is_eap,
)

response = self.do_request(
{
"field": [
"cache.item_size",
"measurements.cache.item_size",
"messaging.message.body.size",
"measurements.messaging.message.body.size",
],
"project": self.project.id,
"dataset": self.dataset,
}
)

assert response.data["data"] == [
{
"id": mock.ANY,
"project.name": self.project.slug,
"cache.item_size": 1.0,
"measurements.cache.item_size": 1.0,
"measurements.messaging.message.body.size": 2.0,
"messaging.message.body.size": 2.0,
},
]

assert response.data["meta"]["fields"] == {
"id": "string",
"project.name": "string",
"cache.item_size": "size",
"measurements.cache.item_size": "size",
"measurements.messaging.message.body.size": "size",
"messaging.message.body.size": "size",
}

assert response.data["meta"]["units"] == {
"id": None,
"project.name": None,
"cache.item_size": "byte",
"measurements.cache.item_size": "byte",
"measurements.messaging.message.body.size": "byte",
"messaging.message.body.size": "byte",
}

def test_simple_measurements(self):
keys = [
("app_start_cold", "duration", "millisecond"),
("app_start_warm", "duration", "millisecond"),
("frames_frozen", "number", None), # should be integer but keeping it consistent
("frames_frozen_rate", "percentage", None),
("frames_slow", "number", None), # should be integer but keeping it consistent
("frames_slow_rate", "percentage", None),
("frames_total", "number", None), # should be integer but keeping it consistent
("time_to_initial_display", "duration", "millisecond"),
("time_to_full_display", "duration", "millisecond"),
("stall_count", "number", None), # should be integer but keeping it consistent
("stall_percentage", "percentage", None),
("stall_stall_longest_time", "number", None),
("stall_stall_total_time", "number", None),
("cls", "number", None),
("fcp", "duration", "millisecond"),
("fid", "duration", "millisecond"),
("fp", "duration", "millisecond"),
("inp", "duration", "millisecond"),
("lcp", "duration", "millisecond"),
("ttfb", "duration", "millisecond"),
("ttfb.requesttime", "duration", "millisecond"),
("score.cls", "number", None),
("score.fcp", "number", None),
("score.fid", "number", None),
("score.inp", "number", None),
("score.lcp", "number", None),
("score.ttfb", "number", None),
("score.total", "number", None),
("score.weight.cls", "number", None),
("score.weight.fcp", "number", None),
("score.weight.fid", "number", None),
("score.weight.inp", "number", None),
("score.weight.lcp", "number", None),
("score.weight.ttfb", "number", None),
("cache.item_size", "size", "byte"),
("messaging.message.body.size", "size", "byte"),
("messaging.message.receive.latency", "duration", "millisecond"),
("messaging.message.retry.count", "number", None),
]

self._test_simple_measurements(keys)

0 comments on commit 950a9db

Please sign in to comment.