feat(relay): Emit empty alert metrics config #51788
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #51788 +/- ##
=======================================
Coverage 81.30 81.30
=======================================
Files 4899 4899
Lines 205623 205671 +48
Branches 11051 11051
=======================================
+ Hits 167178 167205 +27
- Misses 38200 38221 +21
Partials 245 245
|
|
|
||
| class RuleConditionInner(TypedDict): | ||
| op: Literal["eq", "gt", "gte"] | ||
| RuleCondition = Union["LogicalRuleCondition", "ComparingRuleCondition", "NotRuleCondition"] |
There was a problem hiding this comment.
Since most of these types are extremely similar and separate types do not give us a benefit on typed dicts, I'd opt for readability with fewer types.
How would you propose to write it?
There was a problem hiding this comment.
Maybe with Protocol but not strongly opinionated. My suggestion comes from me not liking types coming from strings this way.
| class MetricExtractionConfig(TypedDict): | ||
| version: int | ||
| metrics: List[MetricSpec] |
There was a problem hiding this comment.
Are we postponing the addition of tags here?
There was a problem hiding this comment.
Keeping it simple at this point since it is neither implemented in relay (just the schema). This can be added once the functionality is used.
| field: NotRequired[str] | ||
| value: NotRequired[str] |
There was a problem hiding this comment.
Should we add a comment that one of them is required?
There was a problem hiding this comment.
Generally we should add more comments here describing the types. I initially kept it plain since I didn't want to duplicate too much from relay's typing.
Since I'm gonna add tests, I'll also add a few comments explaining this.
Suspect IssuesThis pull request has been deployed and Sentry has observed the following issues:
Have questions? Reach out to us in the #proj-github-pr-comments channel. Did you find this useful? React with a 👍 or 👎 |
|
PR reverted: 03769a5 |
This reverts commit aa81d5c. Co-authored-by: wedamija <6288560+wedamija@users.noreply.github.com>
|
|
||
|
|
||
| def get_metric_extraction_config(project: Project) -> Optional[MetricExtractionConfig]: | ||
| if not features.has("organizations:metrics-extraction", project.organization): |
There was a problem hiding this comment.
This is the wrong feature flag.
Closes #51445