Skip to content

feat(rpc): enable extrapolation by default#80829

Merged
wmak merged 6 commits into
masterfrom
wmak/feat/enable-extrapolation-by-default
Nov 18, 2024
Merged

feat(rpc): enable extrapolation by default#80829
wmak merged 6 commits into
masterfrom
wmak/feat/enable-extrapolation-by-default

Conversation

@wmak

@wmak wmak commented Nov 15, 2024

Copy link
Copy Markdown
Member
  • This enables extrapolation by default for the rpc queries
  • This fixes a bug where the events endpoint wasn't actually returning
    the result... also re-xfails a bunch of the tests

wmak added 2 commits November 14, 2024 14:51
- This enables extrapolation by default for the rpc queries
- This fixes a bug where the events endpoint wasn't actually returning
  the result... also rexfails a bunch of the tests
@wmak wmak requested a review from a team November 15, 2024 17:07
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 15, 2024
Comment thread src/sentry/search/eap/columns.py Outdated
@dataclass(frozen=True)
class ResolvedColumn(ResolvedAttribute):
# The internal rpc alias for this column
internal_name: str = ""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this default required for some reason? Can't you leave it unspecified and it'll be a required arg when initializing an instance?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmmm good point, my original thinking was that if its not set "" is better than error, but found kw_only for dataclass, because otherwise ResolvedColumn gets mad that internal_name follows default args

Comment thread src/sentry/search/eap/columns.py Outdated
@dataclass(frozen=True)
class ResolvedFunction(ResolvedAttribute):
# The internal rpc alias for this column
internal_name: Function.ValueType = Function.FUNCTION_UNSPECIFIED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same thing here regarding the default value.

Comment thread src/sentry/search/eap/columns.py Outdated

@property
def proto_type(self) -> AttributeKey.Type.ValueType:
"""The rpc always returns functions as floats"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be good to clarify that this is the case even for count()

"count()": 2,
"count(span.duration)": 2,
"count(tags[foo, number])": 1,
"count(tags[foo, number])": 2,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks wrong. Only 1 of the spans have a numeric attribute named foo. Why did it change to 2?

@codecov

codecov Bot commented Nov 15, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #80829       +/-   ##
===========================================
+ Coverage   57.78%   78.42%   +20.63%     
===========================================
  Files        6941     7211      +270     
  Lines      304363   319756    +15393     
  Branches    41979    44012     +2033     
===========================================
+ Hits       175873   250765    +74892     
+ Misses     123995    62603    -61392     
- Partials     4495     6388     +1893     

un xfail test
@wmak wmak changed the title Wmak/feat/enable extrapolation by default feat(rpc): enable extrapolation by default Nov 18, 2024
@wmak wmak merged commit 702c500 into master Nov 18, 2024
@wmak wmak deleted the wmak/feat/enable-extrapolation-by-default branch November 18, 2024 17:34
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants