feat(canonical): Add a shim for canonical keys #8789

jan-auer · 2018-06-20T14:17:56Z

This PR migrates the legacy "sentry.interfaces.*" event data keys to their new canonical representation, i.e. short names. Only canonical names will be stored, but access is still possible via the legacy names in the entire codebase. This includes ingestion and processing.

A full list of renames is attached at the bottom.

I tried to keep this PR to a minimum, with more refactoring to happen afterwards. Especially EventManager code needs some serious clean up and I'd like to walk through the code base to remove every mention of "sentry.interfaces.*".

Mapping Types

The conversion is mostly handled by two shim types:

CanonicalKeyDict: A mutable mapping based a dict that will only store canonical keys internally. Its constructor already converts and deduplicates all legacy keys. It does not synchronize with its input data.
CanonicalKeyView: A read-only view on an underlying mapping. When iterating or accessing keys, it internally checks for the legacy name as well as the canonical name in both directions. Additionally, it guarantees to preserve order for ordered data structures. Length and iteration operate on deduplicated canonical keys. For performance reasons, __len__ is computed in the constructor, all other methods will operate on the underlying data.

Both wrappers prefer the canonical key in case both canonical and legacy keys are present in the input, regardless of the order. This also holds for OrderedDicts.

Event Model Changes

The event model has been changed to now have two properties:

event.node_data: An instance of NodeData that was originally stored at event.data. It contains the raw mutable data dict. Apart from event creation and the bind_nodes utility, it should never be used directly.
event.data: A memoized read-only CanonicalKeyView on node_data. Since this is used by most of the Sentry code base, including plugins that operate on Event models, they continue to work with their legacy keys. Mutation of the event data should not have been allowed before, with the only exception of the EventManager itself.

Warning: Since event.data is memoized, all calls to Event.objects.bind_nodes(event, 'node_data') must be made before the first access of event.objects. Otherwise, len(event) will return an invalid value. At the moment, this is the case in the entire codebase.

API and EventManager Changes

The EventManager used to rely on mutation of the data dict passed into its constructor. This is no longer the case, with the caveat that one must use the return value of normalize now. Apart from tests, there was only one affected location inside LazyData which was already updated in #8774.

manager = EventManager(data)
data = manager.normalize()

The EventManager now uses a mutable CanonicalKeyDict to store data internally and returns it from normalize. This value is assumed directly by LazyData and thus carries through the entire store API code, allowing it and plugins to continue using legacy keys. Finally, the EventManager sets it directly into the Event's NodeData. This is the only place where node_data does not contain a raw dict.

Also, some code like get_path or the schema validation had to be updated to accommodate a non-dict data, either by using isinstance(data, collections.Mapping) or by accessing data.data (the internal raw dict).

Processing and Store Changes

These jobs load event data without the Event model from the cache. Their entry points have been updated to wrap the returned data dict in a mutable CanonicalKeyDict. Since none of these jobs operates on Event models, no further changes were necessary.

Note that data no longer passes isinstance(data, dict).

Warning: To ensure a smooth rollout, save and processing workers should be deployed before web. Otherwise, incompatible data with canonical keys might reach old workers.

Next Steps

I'd not like this to go out right away because it has a high chance of breaking a lot. Ideally, we could deploy a small number of workers first before making the full change.

Validate assumptions and implications described above
Verify that plugins continue to work
Deploy save / process workers
Deploy web
Follow-up: Clean up / refactor all uses of "sentry.interfaces.*"

Renamed Keys

For reference, this is the mapping from legacy keys to canonical keys:

sentry.interfaces.Exception -> exception,
sentry.interfaces.Message -> logentry,
sentry.interfaces.Stacktrace -> stacktrace,
sentry.interfaces.Template -> template,
sentry.interfaces.Query -> query,
sentry.interfaces.Http -> request,
sentry.interfaces.User -> user,
sentry.interfaces.Csp -> csp,
sentry.interfaces.AppleCrashReport -> applecrashreport,
sentry.interfaces.Breadcrumbs -> breadcrumbs,
sentry.interfaces.Contexts -> contexts,
sentry.interfaces.Threads -> threads,
sentry.interfaces.DebugMeta -> debug_meta,

jan-auer · 2018-06-20T15:00:49Z

@mattrobenolt @mitsuhiko I know it's kind of messy. If you're looking for a place to start reviewing, have a look at coreapi, EventManager, canonical.py, or store.py. Please also double-check if my view code matches your expectations of performance.

mitsuhiko · 2018-06-20T15:04:55Z

src/sentry/utils/samples.py

@@ -182,7 +184,7 @@ def load_data(platform, default=None, timestamp=None, sample_name=None):

    # Make breadcrumb timestamps relative to right now so they make sense
    breadcrumbs = data.get('sentry.interfaces.Breadcrumbs')
-    if breadcrumbs is not None:
+    if breadcrumbs is not None and 'values' in breadcrumbs:


It this a change that creeped in or does this PR change behavior WRT to values?

That was a defensive change to keep behavior of the cocoa ingestion test the same. cocoa.json contains a key breadcrumbs: [ ... ], which was not touched by the old code and would error with the new one. So instead of fixing this code, I chose to rather keep the test behavior the same.

Is the cocoa dump the only one that does not have canonical data for value use? If that's the case I rather have the data fixed than special handle this here.

Sure thing. Will change the visual diff though.

mitsuhiko · 2018-06-20T15:06:02Z

src/sentry/api/endpoints/event_apple_crash_report.py

@@ -36,7 +36,7 @@ def get(self, request, event_id):

        self.check_object_permissions(request, event.group)

-        Event.objects.bind_nodes([event], 'data')
+        Event.objects.bind_nodes([event], 'node_data')


This is going to be an issue. I much rather change bind_nodes so that it accepts the db column in addition to the descriptor and not change all calls to bind_nodes. Otherwise we need to touch some other stuff as well (hipchat-ac plugin though we can delete that, getsentry commands).

Can you clarify what you mean with db column vs descriptor please?

Reading the other comment, I think I get it now. That would mean that everyone accessing data would have to change to a new field then, instead of continuing to use event.data. I'm afraid that'd be a higher risk of breaking something.

We could probably also patch this in Event.objects.bind_nodes and add a special case.

@jan-auer i was thinking of letting bind_nodes also take the db_column into account as a fallback.

mitsuhiko · 2018-06-20T15:09:46Z

src/sentry/models/event.py

@@ -106,8 +112,9 @@ def get_event_metadata(self):
        etype = self.data.get('type', 'default')
        if 'metadata' not in self.data:
            # TODO(dcramer): remove after Dec 1 2016


note for @mattrobenolt: the comment is a lie. Cannot be removed yet.

Should I remove it? cc @dcramer - right now the handling of event.message is quite messy and hard to understand. @mitsuhiko and I were thinking about cleaning this up soon, but we'd need some input on which way to go.

mitsuhiko · 2018-06-20T15:10:41Z

src/sentry/models/rawevent.py

@@ -19,11 +21,12 @@ class RawEvent(Model):
    project = FlexibleForeignKey('sentry.Project')
    event_id = models.CharField(max_length=32, null=True)
    datetime = models.DateTimeField(default=timezone.now)
-    data = NodeField(
+    node_data = NodeField(


to mirror the comment above: we might want to keep the field under the original name since it affects bind_nodes. Additionally if we do this change I believe we need to reflect this in a dummy db migration.

Indeed, forgot to run migrations. I'll wait on a decision whether we should rename this or not. Regarding bind_nodes: If we do not rename this field, we'll have to come up with a new name for the accessor attribute and change every access to it. If we keep renaming it to node_data but don't want to change calls to bind_nodes, we'd have to look up the model's attribute name from the db_column (i.e. 'data' -> 'node_data'). There's probably a mapping somewhere in the meta model, but it sounds a bit strange to me tbh.

mitsuhiko · 2018-06-20T15:14:01Z

tests/sentry/test_event_manager.py

-        assert data['sentry.interfaces.User'] == {'id': '1'}
-        assert 'user' not in data
+        assert data['user'] == {'id': '1'}
+        assert 'sentry.interfaces.User' not in data.keys()


Why is keys() necessary here? This is an odd statement and if it has a purpose a comment should clarify what it checks against the new list instead of the container itself.

Because __contains__ will say true if you ask data directly.

Can you add a comment clarifying this? This will likely to be picked up by a linter sooner or later and someone is going to "fix" it.

jan-auer · 2018-06-20T15:35:45Z

This diff shows an example where the old undefined behavior and the new defined behavior are different: The example JSON contains both "user" and "sentry.interfaces.User": https://percy.io/getsentry/sentry/builds/804476

bretthoerner · 2018-06-27T16:38:48Z

Only canonical names will be stored, but access is still possible via the legacy names in the entire codebase.

I don't have any review feedback, but I'm glad this came up (in the geo PR)... because this will effect Snuba and we need to update to be forwards compatible before this is merged since it consumes from Kafka and lives in its own codebase. If I'm reading correctly the interface keys in the JSON will actually be changed.

It should only take me a minute to update Snuba, but we probably need some way to ensure we inform downstream consumers (since we'll probably have more in the future).

mitsuhiko · 2018-06-27T16:42:46Z

@bretthoerner which are the downstream consumers?

bretthoerner · 2018-06-27T16:50:33Z

Snuba is the only existing downstream consumer, but if the keys suddenly changed to (for example) exception our code would have stopped working and inserted nulls into ClickHouse.

I just think we'll have more Kafka consumers in the future, but even if it's just Snuba... ~~I don't think anyone knew~~ I didn't know to update our Kafka consumer for this. Maybe someone else knew, but I don't see any Snuba tickets related to this and I've done most of the consumer code.

dcramer · 2018-06-27T16:51:53Z

This is probably a good reason to say we lock in versions and dont change these keys in the future.

mitsuhiko · 2018-06-27T16:51:59Z

Why does snuba use the raw format and not the json export? I don’t think we noticed anyone using the raw data dict. Where does that happen?

…

On Wed, Jun 27, 2018 at 6:50 PM Brett Hoerner ***@***.***> wrote: Snuba is the only existing downstream consumer, but if the keys suddenly changed to (for example) exception our code would have stopped working and inserted nulls into ClickHouse. I just think we'll have more Kafka consumers in the future, but even if it's just Snuba... I don't think anyone knew to update our Kafka consumer for this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8789 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAc5Ol-gpMFf7bgmF4BIebHmdLa43Vjks5uA7fagaJpZM4UvXYS> .

bretthoerner · 2018-06-27T16:54:12Z

Maybe I'm misunderstanding and the keys aren't changing?

We publish the event.data.data to Kafka and do a plain json.loads of whatever that is. Are you maintaining the old keys in there forever?

https://github.com/getsentry/getsentry/blob/5121257164ea45873cf271d5a80e61c63ca0237c/getsentry/processing.py#L55-L66

mitsuhiko · 2018-06-27T16:56:55Z

No. You are correct this will change. However more annoyingly this format turns out to never haven been stable. All the rest uses the json export. This is double annoying because this dependency was missed and we were operating under the assumption that we are flexible in chaning this format. I suppose changing to what data forwarding plugins do is not an option?

…

On Wed, Jun 27, 2018 at 6:54 PM Brett Hoerner ***@***.***> wrote: Maybe I'm misunderstanding and the keys aren't changing? We publish the event.data.data to Kafka and do a plain json.loads of whatever that is. Are you maintaining the old keys in there forever? https://github.com/getsentry/getsentry/blob/5121257164ea45873cf271d5a80e61c63ca0237c/getsentry/processing.py#L55-L66 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8789 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAc5J5yGibmquZj6u-o1cbSWmkxHDzdks5uA7i0gaJpZM4UvXYS> .

Related to getsentry/sentry#8789 This is the bare minimum we need so Snuba is forward-compatible with events it may see going forward. There may be other coordination work or schemas we share in the future, but this is here so we don't block process on canonicalization.

jan-auer · 2018-06-28T07:03:32Z

@bretthoerner You could use something like CanonicalKeyDict on your side, too.

mitsuhiko · 2018-07-05T21:28:12Z

I changed this back to bind_nodes('data') by giving the event a custom manager.

mitsuhiko · 2018-07-19T22:51:10Z

@jan-auer there is absolutely no way to make the column rename work. As a result i had to go back to event.data being the NodeField itself. The tests pass but I did not actually verify that the data persisted is the correct one. I want to verify this tomorrow.

mattrobenolt

🤷‍♂️

jan-auer self-assigned this Jun 20, 2018

jan-auer added the Impact: Large label Jun 20, 2018

jan-auer requested review from mattrobenolt, mitsuhiko and dcramer June 20, 2018 14:57

jan-auer requested a review from JTCunning June 20, 2018 15:04

mitsuhiko reviewed Jun 20, 2018

View reviewed changes

mitsuhiko removed the request for review from JTCunning June 20, 2018 15:51

jan-auer referenced this pull request Jun 27, 2018

Add interface.

95e4128

bretthoerner mentioned this pull request Jun 27, 2018

Respect and prefer new canonical forms of interface keys. getsentry/snuba#101

Merged

mitsuhiko force-pushed the feat/canonical-keys branch 4 times, most recently from 3470fde to c223cff Compare July 18, 2018 17:08

mitsuhiko force-pushed the feat/canonical-keys branch from c142d09 to 2bb2017 Compare July 20, 2018 09:35

mitsuhiko and others added 24 commits July 24, 2018 18:33

feat: Added config key to force legacy format

1790cca

test: Added test for legacy keyformat

69a66f1

ref: Allow data as alias for node_data

5d99159

ref: Keep the original bind_nodes call

3cdc757

fix: Fixed RawEvent bind_nodes code

3e2e7fe

fix: Implement CanonicalKeyView.__iter__ correctly

6ba7b68

fix: Implement CanonicalKeyView.__iter__ correctly

3114128

feat: Pickle out data for Event/NodeData in legacy format

d888448

fix: Fix a linter error

67a78d0

fix: Only define __getstate__ and not reduce for NodeData

d567b51

test: Adopted a now broken test

40307ef

fix: Make the pickle format of event futher backwards compatible

9950076

fix: Downgrade canonical wrappers to dicts

aba1b41

fix: Fixed a test that broke after normalizing in nodestore

3b906bf

fix: Fixed a few potentially problematic pickle usages

ab5b731

test: Added a test for Event.as_dict returning new style keys

eeb2790

fix: Serialize dicts on the way to json cache and not canonical wrappers

32605c7

fix: Fix test and implementation for event pickling

1c2caab

fix: Fixed a linter error only showing up in getsentry

8cc6609

fix: Downgrade data dictionaries to snuba/kafka

7b8603a

ref: node_data -> data as we can otherwise not support legacy workers

9ac8f85

fix: Fixed some linter errors

098874d

feat: Make the canonical pickling more flexible

8886a94

fix: Do not persist event interfaces

e3738fd

jan-auer force-pushed the feat/canonical-keys branch from 3eac257 to e3738fd Compare July 24, 2018 16:34

mattrobenolt approved these changes Jul 24, 2018

View reviewed changes

jan-auer merged commit 4204ff5 into master Jul 24, 2018

jan-auer deleted the feat/canonical-keys branch July 24, 2018 16:53

tkaemming mentioned this pull request Aug 3, 2018

fix(tests): Return Snuba tests to a workable state #9317

Merged

github-actions bot locked and limited conversation to collaborators Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(canonical): Add a shim for canonical keys #8789

feat(canonical): Add a shim for canonical keys #8789

jan-auer commented Jun 20, 2018 •

edited

jan-auer commented Jun 20, 2018

mitsuhiko Jun 20, 2018

jan-auer Jun 20, 2018

mitsuhiko Jun 20, 2018

jan-auer Jun 20, 2018

mitsuhiko Jun 20, 2018

jan-auer Jun 20, 2018

jan-auer Jun 20, 2018

mitsuhiko Jun 20, 2018

mitsuhiko Jun 20, 2018

jan-auer Jun 20, 2018

mitsuhiko Jun 20, 2018

jan-auer Jun 21, 2018

mitsuhiko Jun 20, 2018

jan-auer Jun 20, 2018

mitsuhiko Jun 20, 2018

jan-auer commented Jun 20, 2018

bretthoerner commented Jun 27, 2018 •

edited

mitsuhiko commented Jun 27, 2018 •

edited

bretthoerner commented Jun 27, 2018 •

edited

dcramer commented Jun 27, 2018

mitsuhiko commented Jun 27, 2018 via email

bretthoerner commented Jun 27, 2018

mitsuhiko commented Jun 27, 2018 via email

jan-auer commented Jun 28, 2018

mitsuhiko commented Jul 5, 2018

mitsuhiko commented Jul 19, 2018

mattrobenolt left a comment

feat(canonical): Add a shim for canonical keys #8789

feat(canonical): Add a shim for canonical keys #8789

Conversation

jan-auer commented Jun 20, 2018 • edited

Mapping Types

Event Model Changes

API and EventManager Changes

Processing and Store Changes

Next Steps

Renamed Keys

jan-auer commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer commented Jun 20, 2018

bretthoerner commented Jun 27, 2018 • edited

mitsuhiko commented Jun 27, 2018 • edited

bretthoerner commented Jun 27, 2018 • edited

dcramer commented Jun 27, 2018

mitsuhiko commented Jun 27, 2018 via email

bretthoerner commented Jun 27, 2018

mitsuhiko commented Jun 27, 2018 via email

jan-auer commented Jun 28, 2018

mitsuhiko commented Jul 5, 2018

mitsuhiko commented Jul 19, 2018

mattrobenolt left a comment

Choose a reason for hiding this comment

jan-auer commented Jun 20, 2018 •

edited

bretthoerner commented Jun 27, 2018 •

edited

mitsuhiko commented Jun 27, 2018 •

edited

bretthoerner commented Jun 27, 2018 •

edited