ref(spans-buffer): Clean up ZSET code#107863
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
evanh
left a comment
evanh
left a comment
There was a problem hiding this comment.
Looks good, pending the rollout to ST.
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Please fix the way the options are cleaned up
| # ZSET to SET migration options. | ||
| register( | ||
| "spans.buffer.write-to-zset", | ||
| default=False, | ||
| flags=FLAG_PRIORITIZE_DISK | FLAG_AUTOMATOR_MODIFIABLE, | ||
| ) | ||
| register( | ||
| "spans.buffer.write-to-set", | ||
| default=True, | ||
| flags=FLAG_PRIORITIZE_DISK | FLAG_AUTOMATOR_MODIFIABLE, | ||
| ) | ||
| register( | ||
| "spans.buffer.read-from-set", | ||
| default=True, | ||
| flags=FLAG_PRIORITIZE_DISK | FLAG_AUTOMATOR_MODIFIABLE, | ||
| ) | ||
|
|
There was a problem hiding this comment.
You cannot remove options this way.
You have to unset them first from option automator.
Which means:
- remove the usage of the option in code, but not the option registration. In alternative you can expedite the process by changing the default to the behavior you want.
- remove the option from option automator
- only then you can remove the option from here
There was a problem hiding this comment.
Did you apply the changes to the add-buffer.lua manually or did you copy over the content of add-buffer-set.lua ?
There was a problem hiding this comment.
I copied over the content of add-buffer-set.lua with git mv but I also made changes to the metric names (remove the set_ prefix) as well as documentation changes.
| def compare_metrics( | ||
| zset_metrics: list[EvalshaData], | ||
| set_metrics: list[EvalshaData], | ||
| ) -> None: | ||
| """ | ||
| Reports the difference (SET - ZSET) between metrics. | ||
|
|
||
| SET metrics are expected to have a "set_" prefix (e.g., "set_redirect_depth"). | ||
| Special cases are handled via ZSET_TO_SET_KEY_MAPPING. | ||
| """ | ||
| differences: dict[str, tuple[float, float, float, float]] = {} | ||
|
|
||
| for zset_evalsha, set_evalsha in zip(zset_metrics, set_metrics): | ||
| set_dict: dict[str, float] = { | ||
| raw_key.decode("utf-8"): value for raw_key, value in set_evalsha | ||
| } | ||
|
|
||
| for raw_key, zset_value in zset_evalsha: | ||
| zset_key = raw_key.decode("utf-8") | ||
| set_key = ZSET_TO_SET_KEY_MAPPING.get(zset_key, f"set_{zset_key}") | ||
|
|
||
| if set_key not in set_dict: | ||
| continue | ||
|
|
||
| diff = set_dict[set_key] - zset_value | ||
|
|
||
| if zset_key not in differences: | ||
| differences[zset_key] = (diff, diff, diff, 1.0) | ||
| else: | ||
| differences[zset_key] = ( | ||
| min(differences[zset_key][0], diff), | ||
| max(differences[zset_key][1], diff), | ||
| differences[zset_key][2] + diff, | ||
| differences[zset_key][3] + 1.0, |
There was a problem hiding this comment.
It's OK to remove it, I will update my code accordingly.
There was a problem hiding this comment.
If we want to do EVALSHA comparisons in the future, we can keep this function. I can modify this into something more generic (i.e. comparing between metrics_A: list[EvalshaData] and metrics_B: list[EvalshaData]
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Cleans up old ZSET code to finish off spans buffer migration to unsorted sets.
3 participants
Cleans up old ZSET code to finish off spans buffer migration to unsorted sets.