Skip to content

feat(file-io): Handle duplicate file-io spans#43955

Merged
wmak merged 3 commits into
masterfrom
wmak/feat/handle-duplicate-fileio-spans
Feb 23, 2023
Merged

feat(file-io): Handle duplicate file-io spans#43955
wmak merged 3 commits into
masterfrom
wmak/feat/handle-duplicate-fileio-spans

Conversation

@wmak

@wmak wmak commented Feb 1, 2023

Copy link
Copy Markdown
Member
  • Today when there are duplicate file spans blocking main thread, the number of file-io spans will change the fingerprinting. This change updates this so that regardless of the number of duplicates the fingerprint does not change

- Today when there are duplicate file spans blocking main thread, the
  number of file-io spans will change the fingerprinting. This change
  updates this so that regardless of the number of duplicates the
  fingerprint does not change

@narsaynorath narsaynorath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That failing test seems related to this change, it makes sense that the hashes are different now and need updating

problem = self.find_problems(event)[0]
assert problem.title == "File IO on Main Thread"

def test_duplicate_calls_do_not_change_callstackc(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def test_duplicate_calls_do_not_change_callstackc(self):
def test_duplicate_calls_do_not_change_callstack(self):

@wmak

wmak commented Feb 1, 2023

Copy link
Copy Markdown
Member Author

That failing test seems related to this change, it makes sense that the hashes are different now and need updating

🤔 actually I intentionally wrote this in a way to not change existing fingerprints (otherwise customers would see a bunch of new duplicates) But this test passed locally... going to look into it

Comment on lines +1386 to +1387
call_stack_strings = set()
overall_stack = set()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this ever cause an issue as sets don't preserve order?
I.e you'll create new fingerprints for issues that already exists.

@wmak wmak Feb 1, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh good catch, i'll probably have to remove duplicates from the list instead of just using a set 👍

Probably why it flaked earlier too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup! Or just check if not in before insertion

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok but hear me out, what about:
sorted(set(list), lambda x: list.index(x))

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@wmak wmak merged commit 48fdeea into master Feb 23, 2023
@wmak wmak deleted the wmak/feat/handle-duplicate-fileio-spans branch February 23, 2023 15:46
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants