Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicates in breadcrumbs #8039

Merged
merged 1 commit into from
May 30, 2024

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented May 27, 2024

Introduced a new method _format_breadcrumbs to handle the formatting of breadcrumbs in SentryReporter. This includes removing duplicates and ordering by timestamp. The logic for this has been extracted from the _before_send method.

An event example: https://sentry.tribler.org/organizations/tribler/issues/2658/events/b42dc2acdc184c709ab8f9a60367c23e/

This is a post-work of

@drew2a drew2a self-assigned this May 27, 2024
@drew2a drew2a added this to the 7.15.0 milestone May 27, 2024
@drew2a drew2a force-pushed the feature/uniq_breadcrumbs branch from 171b4aa to 50c3f4f Compare May 27, 2024 12:13
@drew2a drew2a requested a review from kozlovsky May 27, 2024 13:36
@drew2a drew2a marked this pull request as ready for review May 27, 2024 13:36
@drew2a drew2a requested a review from a team May 27, 2024 13:36
src/tribler/core/sentry_reporter/sentry_tools.py Outdated Show resolved Hide resolved
values_viewed.add(value)

return result
distinct = dict((getter(d), d) for d in list_of_dict)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach, the keys are ordered according to the order as they first appear in the list, but the item stored is the last item corresponding to this key. That is, the latest item with the same key overrides the first item but keeps the position of the first item. If we consider distinct_by a general purpose function, in some use cases, it can lead to a confusing order (depending on what is an "item" and what is a "key") because it can break expectations of the function's user, that probably expects that the first item is stored, not the last one, but for breadcrumbs, it should work fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but my reasoning is that a user should not rely on expectations about ordering if it is not explicitly written in the documentation or reflected in the function design. In our case, what is guaranteed is that the values will be unique based on the given key (getter).

However, we can use the following logic:

    distinct = {}
    for item in items:
        key = getter(item)
        if key not in distinct:
            distinct[key] = item
    return list(distinct.values())

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

src/tribler/core/sentry_reporter/sentry_tools.py Outdated Show resolved Hide resolved
@drew2a drew2a force-pushed the feature/uniq_breadcrumbs branch from 898c72f to 4797788 Compare May 29, 2024 10:49
@drew2a drew2a merged commit 958b655 into Tribler:main May 30, 2024
20 checks passed
@drew2a drew2a deleted the feature/uniq_breadcrumbs branch May 30, 2024 11:08
@drew2a
Copy link
Contributor Author

drew2a commented May 30, 2024

@kozlovsky thank you for the review 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants