Skip to content

fix(hc): Change send_incident_alert_notification to take raw JSON#54592

Merged
RyanSkonnord merged 2 commits into
masterfrom
incident-attachment-rpc-serializability
Aug 14, 2023
Merged

fix(hc): Change send_incident_alert_notification to take raw JSON#54592
RyanSkonnord merged 2 commits into
masterfrom
incident-attachment-rpc-serializability

Conversation

@RyanSkonnord

Copy link
Copy Markdown
Contributor

Resolve a TODO comment to remove the use of Mapping[str, Any] as a parameter. The Any would be a problem because it can't be cleanly represented in a JSON schema and won't stop callers from passing non-serializable values.

Because the incident attachment is treated as an arbitrary JSON blob, have the RPC method take an arbitrary string containing JSON, which is loaded on the other side. The string is presumed to contain valid JSON syntax, and will cause a server-side exception otherwise.

Resolve a TODO comment to remove the use of `Mapping[str, Any]` as a
parameter. The `Any` would be a problem because it can't be cleanly
represented in a JSON schema and won't stop callers from passing
non-serializable values.

Because the incident attachment is treated as an arbitrary JSON blob,
have the RPC method take an arbitrary string containing JSON, which is
loaded on the other side. The string is presumed to contain valid JSON
syntax, and will cause a server-side exception otherwise.
@RyanSkonnord RyanSkonnord requested a review from a team as a code owner August 10, 2023 21:31
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 10, 2023
@RyanSkonnord

Copy link
Copy Markdown
Contributor Author

Passing arbitrary JSON blobs around is never my first choice, but I think it's acceptable in this case because the pre-RPC code is already treating it as such -- creating it in build_incident_attachment and passing it unmodified until it becomes the AppPlatformEvent.data attribute.

The alternative would be to create an RpcIncidentAttachment model, which would be a win for the strictness/informativeness of the RPC schema, but would also be a lot of code for not much benefit. The metric_alert field would require a nested representation of the Incident object, which itself would require a nested representation of AlertRule.

To give you a sense of scale, here's an example of the JSON object from a unit test:

{
  "metric_alert": {
    "id": "1",
    "identifier": "1",
    "organization_id": "4552430653538320",
    "projects": ["bar"],
    "alert_rule": {
      "id": "2",
      "name": "Real Swift",
      "organization_id": "4552430653538320",
      "status": 0,
      "query_type": 0,
      "dataset": "events",
      "query": "level:error",
      "aggregate": "count()",
      "threshold_type": 0,
      "resolve_threshold": null,
      "time_window": 10.0,
      "environment": null,
      "resolution": 1.0,
      "threshold_period": 1,
      "triggers": [],
      "projects": ["bar"],
      "include_all_projects": false,
      "owner": null,
      "original_alert_rule_id": null,
      "comparison_delta": null,
      "date_modified": "2023-08-10T20:52:08.485958Z",
      "date_created": "2023-08-10T20:52:08.485958Z",
      "created_by": null
    },
    "activities": null,
    "seen_by": null,
    "has_seen": null,
    "status": 2,
    "status_method": 3,
    "type": 2,
    "title": "Nearby Bluegill",
    "date_started": "2023-08-10T20:52:08.485958Z",
    "date_detected": "2023-08-10T20:52:08.485958Z",
    "date_created": "2023-08-10T20:52:08.485958Z",
    "date_closed": null
  },
  "description_text": "1000 events in the last 10 minutes",
  "description_title": "Resolved: Real Swift",
  "web_url": "http://testserver/organizations/baz/alerts/rules/details/2/?alert=1"
}

That's a lot of fields to represent in RpcModel objects for a one-off representation that doesn't want to do anything with them except unpack them back into a raw dict. Plus we'd be depriving the legacy code of the flexibility to treat it as a simple dict, in case anyone wants to add new fields in IncidentSerializer or wherever.

@codecov

codecov Bot commented Aug 10, 2023

Copy link
Copy Markdown

Codecov Report

Merging #54592 (b844335) into master (9bd7dd4) will increase coverage by 0.00%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #54592   +/-   ##
=======================================
  Coverage   79.78%   79.78%           
=======================================
  Files        5000     5000           
  Lines      212244   212244           
  Branches    36162    36162           
=======================================
+ Hits       169330   169335    +5     
+ Misses      37706    37702    -4     
+ Partials     5208     5207    -1     
Files Changed Coverage Δ
src/sentry/rules/actions/notify_event_service.py 73.83% <100.00%> (ø)
...c/sentry/services/hybrid_cloud/integration/impl.py 81.18% <100.00%> (ø)
...entry/services/hybrid_cloud/integration/service.py 80.20% <100.00%> (ø)

... and 5 files with indirect coverage changes

@corps

corps commented Aug 14, 2023

Copy link
Copy Markdown
Contributor

I agree. When in doubt, I think the principle of source of truth rules. Rpc typing makes sense because HC is the producing source of truth of these models in most cases. When we're just passing along values opaque on behalf of other logic, I think it's fair to be more flexible to that logic's design.

@RyanSkonnord RyanSkonnord merged commit a2d7081 into master Aug 14, 2023
@RyanSkonnord RyanSkonnord deleted the incident-attachment-rpc-serializability branch August 14, 2023 22:28
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 30, 2023
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.

3 participants