ref(grouping): Change exception subcomponent order in grouping info #102281
Conversation
github-actions
bot
added
the
Scope: Backend
lobsterkatie
force-pushed
the
kmclb-switch-order-of-exception-subcomponents-in-grouping-info
branch
from
96054d9 to
959dffe
Compare
| """ | ||
| ordered_values: Any = [] | ||
|
|
||
| for component_id in ["type", "value", "ns_error", "stacktrace"]: |
There was a problem hiding this comment.
Nit: I am concerned about the hardcoded list of values not being future-proofed... but recognize that this is a pretty small class definition and we'd probably notice. Still, I think my other suggestion will address this.
| if subcomponent: | ||
| ordered_values.append(subcomponent) | ||
|
|
||
| self.values = ordered_values |
There was a problem hiding this comment.
You know more about grouping than I do — and I'll defer to your expertise — but I worry that we might be dropping some values here. BaseGroupingComponent seems to explicitly support having multiple subcomponents of the same ID (via iter_subcomponents) and this would only preserve the first one.
Instead could we just do:
self.values.sort(
key=lambda subcomponent: {"type": 0, "value": 1, "ns_error": 2, "stacktrace": 3}.get(subcomponent, 999)
)
This would order everything in-place, preserve items with duplicate IDs, and not drop things with unexpected IDs.
There was a problem hiding this comment.
In an abstract sense you're right, but it's not an issue here because exception components can only ever have one copy of any given subcomponent type. (This mirrors the fact that a given error only ever is of one type, only ever has a single error message, etc. Yes, there can be chained exceptions, but each exception in the chain still only has a single type/value/stacktrace/etc.)
This is also only a temporary work-around, to get us from the universe where we do values = [stacktrace_component, type_component, ns_error_component, value_component] when creating the exception component to the universe where we do values = [type_component, value_component, ns_error_component, stacktrace_component] without it being noticeable to the user. (The change here will be visible, of course, but this will get us to the final state visually even before we've finished transitioning under the hood.)
There was a problem hiding this comment.
Coolio, I'll approve — there is a plan to change the original values order (and then we can remove this method)?
There was a problem hiding this comment.
Yup, that's what these two parts in the description were trying to say:
the new grouping config, in which the order of the subcomponents in
ExceptionGroupingComponentwill be changed
and
Once we're fully switched over to the new config, we can delete this shim.
lobsterkatie
deleted the
kmclb-switch-order-of-exception-subcomponents-in-grouping-info
branch
This is a small change to grouping info to prep for the new grouping config, in which the order of the subcomponents in
ExceptionGroupingComponentwill be changed. So that there's no inconsistency between the way the current config's grouping info displays and the way the new config's grouping info will display, this manually reorders the elements when serializing them, to match the new order. Once we're fully switched over to the new config, we can delete this shim.(The reason for the reordering is that currently, stacktrace comes first, with the result that unless the stacktrace is either very short or collapsed, you can't see the error type and value components without scrolling.)