Skip to content

Conversation

@shruthilayaj
Copy link
Member

The preflight was returning duplicated tag key names (we store the same
attribute key in different format in EAP). Dedupe it before making the
second request.

@shruthilayaj shruthilayaj requested a review from a team as a code owner December 12, 2025 22:41
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 12, 2025
Comment on lines 56 to 57
data = self.data_fn(offset=offset, limit=limit)
has_more = data[1] >= limit + 1

This comment was marked as outdated.

# Request 1 more than limit so we can tell if there is another page
data = self.data_fn(offset=offset, limit=limit + 1)
data = self.data_fn(offset=offset, limit=limit)
has_more = data[1] >= limit + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Pagination never detects next page after limit change

The pagination has_more check is broken. Previously data_fn was called with limit + 1 to detect if more results exist, but now it's called with just limit. However, the has_more condition still checks if data[1] >= limit + 1. Since data_fn returns len(request_attrs_list) which is at most limit (due to the slice [offset : offset + limit]), this condition will never be true. As a result, pagination will never show a "next page" link even when more results exist.

Fix in Cursor Fix in Web

# Request 1 more than limit so we can tell if there is another page
data = self.data_fn(offset=offset, limit=limit + 1)
data = self.data_fn(offset=offset, limit=limit)
has_more = data[1] >= limit + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Pagination never detects next page after limit change

The pagination has_more check is broken. Previously data_fn was called with limit + 1 to detect if more results exist, but now it's called with just limit. However, the has_more condition still checks if data[1] >= limit + 1. Since data_fn returns len(request_attrs_list) which is at most limit (due to the slice [offset : offset + limit]), this condition will never be true. As a result, pagination will never show a "next page" link even when more results exist.

Fix in Cursor Fix in Web

@codecov
Copy link

codecov bot commented Dec 12, 2025

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
30553 2 30551 242
View the top 2 failed test(s) by shortest run time
tests.snuba.api.endpoints.test_organization_trace_item_stats.OrganizationTraceItemsStatsEndpointTest::test_custom_limit_parameter
Stack Traces | 15.9s run time
#x1B[1m#x1B[.../api/endpoints/test_organization_trace_item_stats.py#x1B[0m:235: in test_custom_limit_parameter
    assert (
#x1B[1m#x1B[31mE   AssertionError: assert 1 == 2#x1B[0m
#x1B[1m#x1B[31mE    +  where 1 = len({'sentry.custom3': [{'label': 'value3', 'value': 1.0}]})#x1B[0m
tests.snuba.api.endpoints.test_organization_trace_item_stats.OrganizationTraceItemsStatsEndpointTest::test_substring_match_returns_known_public_aliases
Stack Traces | 18.5s run time
#x1B[1m#x1B[.../api/endpoints/test_organization_trace_item_stats.py#x1B[0m:145: in test_substring_match_returns_known_public_aliases
    assert len(response.data["data"]) == 1
#x1B[1m#x1B[31mE   assert 0 == 1#x1B[0m
#x1B[1m#x1B[31mE    +  where 0 = len([])#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@getsantry
Copy link
Contributor

getsantry bot commented Jan 3, 2026

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Jan 3, 2026
sanitized_keys.add(internal_name)

sanitized_keys = sanitized_keys[offset : offset + limit]
sanitized_keys = list(sanitized_keys)[offset : offset + limit]

This comment was marked as outdated.

if key in SPANS_STATS_EXCLUDED_ATTRIBUTES_PUBLIC_ALIAS:
sanitized_keys = set()
for internal_name in internal_alias_attr_keys:
if internal_name in SPANS_STATS_EXCLUDED_ATTRIBUTES_PUBLIC_ALIAS:

This comment was marked as outdated.

for key in attr_keys:
if key in SPANS_STATS_EXCLUDED_ATTRIBUTES_PUBLIC_ALIAS:
for internal_name in internal_alias_attr_keys:
if internal_name in sanitized_keys_set:

This comment was marked as outdated.


sanitized_keys.append(key)
sanitized_keys_set.add(internal_name)
sanitized_keys.append(internal_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Deduplication tracks internal names instead of public aliases

Medium Severity

The deduplication logic adds internal_name to sanitized_keys_set and checks against it, but internal_alias_attr_keys comes from dictionary keys which are already unique. The PR's goal is to deduplicate when different internal names map to the same public_alias, but the set tracks internal_name instead of public_alias. This means multiple internal names mapping to the same public alias will all be included, defeating the deduplication purpose.

Fix in Cursor Fix in Web

@shruthilayaj shruthilayaj merged commit af904ec into master Jan 7, 2026
65 checks passed
@shruthilayaj shruthilayaj deleted the shruthi/dedupe-aliases-before-request branch January 7, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants