sentry

sentry copied to clipboard

The motivation for the check is that breaking the backwards compatibility of an RPC interface can cause errors on production, even if all call sites are updated in the same code change, because the change is rolled out to the control and region silos asynchronously. Introduce a CI check that will warn the developer if they make such a breaking change.

Introduce a sentry rpcschema command that prints an approximate schema, in OpenAPI format, of the RPC services to stdout.

Apr 05 '24 12:04 RyanSkonnord

Bundle Report

Changes will increase total bundle size by 824 bytes :arrow_up:

Apr 09 '24 22:04 codecov[bot]

Codecov Report

Attention: Patch coverage is 63.33333% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 79.68%. Comparing base (e8e01b0) to head (b15a7c8). Report is 86 commits behind head on master.

@@             Coverage Diff             @@
##           master   #68337       +/-   ##
===========================================
- Coverage   91.36%   79.68%   -11.68%     
===========================================
  Files        2861     6419     +3558     
  Lines      178773   287033   +108260     
  Branches    31892    49486    +17594     
===========================================
+ Hits       163334   228735    +65401     
- Misses      15438    57922    +42484     
- Partials        1      376      +375     

Apr 10 '24 21:04 codecov[bot]

@asottile-sentry (CC @bukzor)

Would the overhead of making this a new CI job be acceptable if it runs only on PRs that change code in the directory of Hybrid Cloud RPC services, per https://github.com/getsentry/sentry/pull/68337/commits/b761175c121ba4ffbd9c7bd9a5f8baaf2049eedd?

My understanding of this comment thread is that the concern is about having the new check run as a new job, regardless of which workflow it's in. If I'm wrong and the change from https://github.com/getsentry/sentry/pull/68337/commits/6a5153b45162c4cbb23520d73dbbb2860e32350c was sufficient, let me know.

Apr 15 '24 20:04 RyanSkonnord

@asottile-sentry (CC @bukzor)

Would the overhead of making this a new CI job be acceptable if it runs only on PRs that change code in the directory of Hybrid Cloud RPC services, per b761175?

My understanding of this comment thread is that the concern is about having the new check run as a new job, regardless of which workflow it's in. If I'm wrong and the change from 6a5153b was sufficient, let me know.

new workflows and new jobs are both problematic -- ideally this should get folded into the api schema job that we already have and then there would not be any concerns. running only on HC changes would be "fine" but I think that's impossible to detect due to how tangled our codebase is

Apr 15 '24 20:04 asottile-sentry

running only on HC changes would be "fine" but I think that's impossible to detect due to how tangled our codebase is

I believe

    paths:
      - src/sentry/services/hybrid_cloud/**

should be sufficient, as all our Hybrid Cloud RPC services live in that package (plus a corresponding package in getsentry that would be handled the same way). It would pick up false positives only when we tinker with the base RPC implementation, which is rare. The only part that's spread over the codebase is the calls to the RPCs, which don't need to trigger the check.

Apr 15 '24 21:04 RyanSkonnord

running only on HC changes would be "fine" but I think that's impossible to detect due to how tangled our codebase is

I believe

    paths:
      - src/sentry/services/hybrid_cloud/**

should be sufficient, as all our Hybrid Cloud RPC services live in that package (plus a corresponding package in getsentry that would be handled the same way). It would pick up false positives only when we tinker with the base RPC implementation, which is rare. The only part that's spread over the codebase is the calls to the RPCs, which don't need to trigger the check.

I believe it's not -- those modules can import the rest of the codebase for types, etc. which can be reflected in the api

Apr 16 '24 13:04 asottile-sentry

Sorry for the slow reply; I've been out sick.

I believe it's not -- those modules can import the rest of the codebase for types, etc. which can be reflected in the api

For the most part, they don't. The RPC service method signatures are composed mostly of primitive types and RpcModel subclasses, the latter of which all live in src/sentry/services/hybrid_cloud/**/model paths. Changes to RPC model classes would trigger the check.

If there are any straggler references to types from outside that path which would be reflected in the API, I believe it's limited to a few small details like enum classes -- nothing major like ORM types. If those references are capable of causing rare false negatives, we're okay with this being a "best effort" check.

Apr 23 '24 18:04 RyanSkonnord

I notice the followup item is closed already -- should that get picked up so the traceback stuff can get fixed too?

There are a few more RPC methods that need their return types updated. I'll be following up on those. The work remaining is cataloged in HC-1190

May 14 '24 15:05 markstory

Looks like we need to revert this, it is breaking self-hosted CI and seeing

#16 [ 8/10] RUN sentry help | sed '1,/Commands:/d' | awk '{print $1}' >  /sentry-commands.txt
#16 0.625 Traceback (most recent call last):
#16 0.625   File "/usr/local/bin/sentry", line 5, in <module>
#16 0.625     from sentry.__main__ import main
#16 0.625   File "/usr/local/lib/python3.11/site-packages/sentry/__main__.py", line 1, in <module>
#16 0.625     from .runner.main import main
#16 0.625   File "/usr/local/lib/python3.11/site-packages/sentry/runner/main.py", line 37, in <module>
#16 0.626     for cmd in map(
#16 0.626   File "/usr/local/lib/python3.11/site-packages/sentry/utils/imports.py", line 29, in import_string
#16 0.626     result = _cache[path]
#16 0.626              ~~~~~~^^^^^^
#16 0.626   File "/usr/local/lib/python3.11/site-packages/sentry/utils/imports.py", line 11, in __missing__
#16 0.626     module = __import__(module_name, {}, {}, [class_name])
#16 0.626              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#16 0.626   File "/usr/local/lib/python3.11/site-packages/sentry/runner/commands/rpcschema.py", line 9, in <module>
#16 0.626     from openapi_pydantic import OpenAPI
#16 0.626 ModuleNotFoundError: No module named 'openapi_pydantic'

May 14 '24 17:05 hubertdeng123

PR reverted: 945acb5a9cbd2595f0f40754043edc813ea7b50d

May 14 '24 17:05 getsentry-bot