feat(eap): use eap_items in timeseries endpoint #6934
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidtsuk
force-pushed
the
david/feat/use-eap-items
branch
from
b409c28 to
955fb24
Compare
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
kylemumma
reviewed
Mar 5, 2025
volokluev
reviewed
Mar 5, 2025
| @@ -38,6 +40,160 @@ | |||
| "sentry.end_timestamp", | |||
| } | |||
|
|
|||
| COLLUMN_PREFIX: str = "sentry." | |||
volokluev
reviewed
Mar 5, 2025
Comment on lines
48
to
59
| "sentry.organization_id": [AttributeKey.Type.TYPE_INT], | ||
| "sentry.project_id": [AttributeKey.Type.TYPE_INT], | ||
| "sentry.timestamp": [ | ||
| AttributeKey.Type.TYPE_FLOAT, | ||
| AttributeKey.Type.TYPE_DOUBLE, | ||
| AttributeKey.Type.TYPE_INT, | ||
| AttributeKey.Type.TYPE_STRING, | ||
| ], | ||
| "sentry.trace_id": [ | ||
| AttributeKey.Type.TYPE_STRING | ||
| ], # this gets converted from a uuid to a string in a storage processor | ||
| "sentry.item_id": [AttributeKey.Type.TYPE_STRING], |
There was a problem hiding this comment.
why define a prefix if you won't use it?
There was a problem hiding this comment.
it's being used in the attribute_key_to_expression_eap_items function
There was a problem hiding this comment.
I meant that it's used here too but not explicitly i.e.
/s/"sentry.organization_id"/f"{PREFIX}organization_id"
just a nit
| } | ||
|
|
||
| PROTO_TYPE_TO_ATTRIBUTE_COLUMN: Final[Mapping[AttributeKey.Type.ValueType, str]] = { | ||
| AttributeKey.Type.TYPE_INT: "attributes_int", |
There was a problem hiding this comment.
use floats for TYPE_INT and TYPE_BOOLEAN (cast them in the query)
the attributes_int, attributes_bool are there to reconstruct the original event but we double write them to the float column for aggregations since those are bucketed
Comment on lines
+171
to
+172
| if expression.column.column_name != "attributes_string": | ||
| return expression |
| @@ -240,25 +267,37 @@ def _proto_expression_to_ast_expression(expr: ProtoExpression) -> Expression: | |||
|
|
|||
| def _build_query(request: TimeSeriesRequest) -> Query: | |||
| # TODO: This is hardcoded still | |||
There was a problem hiding this comment.
you can remove this comment I think
| @@ -303,13 +304,14 @@ def _get_count_column_alias( | |||
|
|
|||
| def get_count_column( | |||
| aggregation: AttributeAggregation | AttributeConditionalAggregation, | |||
| attribute_key_to_expression: Callable[[Any], Expression], | |||
There was a problem hiding this comment.
nit: is the typing for this right? It's not really [Any] -> Expression is it?
Comment on lines
185
to
+190
| spans_storage = get_storage(StorageKey("eap_spans")) | ||
| items_storage = get_storage(StorageKey("eap_items")) | ||
| start = BASE_TIME | ||
| messages = [gen_message(start - timedelta(minutes=i)) for i in range(120)] | ||
| write_raw_unprocessed_events(spans_storage, messages) # type: ignore | ||
| write_raw_unprocessed_events(items_storage, messages) # type: ignore |
There was a problem hiding this comment.
I would extract the double writing to its own function and change that across the board rather than writing the double write everywhere
| ) | ||
|
|
||
|
|
||
| def apply_virtual_columns_eap_items( |
There was a problem hiding this comment.
where does this function get called?
There was a problem hiding this comment.
it doesn't get called yet but when I update the trace item table endpoint, it will get used
|
Oh one more thing so we can compare performance: Can you add something that lets us force using the eap_spans table from the client side so we can compare performance? Maybe a special prefix in the referrer or something |
volokluev
reviewed
Mar 5, 2025
| @@ -77,6 +77,9 @@ | |||
|
|
|||
|
|
|||
| def use_eap_items_table(request_meta: RequestMeta) -> bool: | |||
| if request_meta.referrer.startswith("force_use_eap_items_table"): | |||
There was a problem hiding this comment.
I want to force using the OLD table
think about this: I run a timeseries query for some ingested timeframe, if use_eap_items_table is turned on, there's no way to for me to compare that query against the old table.
There was a problem hiding this comment.
I see, I thought you wanted to test it before turning it on
volokluev
approved these changes
Mar 5, 2025
jan-auer
added a commit
that referenced
this pull request
Mar 6, 2025
* master: feat(eap): use eap_items in timeseries endpoint (#6934) chore(tests): Clean up which sentry tests are run on snuba PRs (#6938) Revert "bug: cant use group by with formulas (#6933)" bug: cant use group by with formulas (#6933) fix(test): run more relevant sentry CI tests (#6937) fix(ourlogs): Logs trace item table should support offset (#6936) Revert "fix(eap): Remove column processor preventing the use of an index (#6932)" fix(eap): Remove column processor preventing the use of an index (#6932) feat(eap): Sampled storage materialized views (#6930) feat(capman): Instead of flatly rejecting heavy projects, limit their bytes scanned on the query level (#6929) feat(ourlogs): Add a trace item details endpoint (#6921) Run rustup toolchain install explicitly (#6928) fix(ci): Auth protoc to avoid rate limits (#6927) fix(eap): Set a max execution time for queries (#6925) feat(smart-autocomplete): add migration for smart autocomplete view on eap_items (#6912)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the timeseries endpoint for the eap_spans resolver to use the new eap_items table when the following conditions are met:
use_eap_items_tableruntime config is set to1use_eap_items_table_start_timestamp_seconds(measured in UTC seconds)The tests have also been updated to been run once on the eap_spans table and once on the eap_items table.