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.

Sign up for GitHub

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

Jump to bottom

debuginfo: Improve debuginfo quality checking using metadata #1157

Merged
merged 17 commits into from
Jun 30, 2022
Merged

debuginfo: Improve debuginfo quality checking using metadata #1157

merged 17 commits into from
Jun 30, 2022

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Jun 14, 2022 •
edited
Loading

  • Add source file hash to the metadata file
  • Reduce metadata states
  • Check if metadata is stale
  • Check and handle the file corruption
    • Using a limited buffer

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Fixes #1097

Blocked by #1265

@kakkoyun kakkoyun requested review from metalmatze and brancz June 14, 2022 13:05
brancz
brancz reviewed Jun 14, 2022
View reviewed changes
pkg/debuginfo/store.go Outdated Show resolved Hide resolved
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from ccdbf06 to d78ac67 Compare June 14, 2022 14:32
@brancz
Copy link
Member

brancz commented Jun 14, 2022

if we've tested this, I'm happy to merge this

@kakkoyun
Copy link
Member Author

It's not fully tested. I have parked it for now, and I'll return to it soonish.

@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch 2 times, most recently from 1a7b90e to be13846 Compare June 16, 2022 13:30
@kakkoyun
Copy link
Member Author

In subsequent PRs. We should add integration tests for debuginfo.Store.Exists and debuginfo.Store.Upload using the debuginfo.Client.

@kakkoyun kakkoyun marked this pull request as ready for review June 16, 2022 13:33
@kakkoyun kakkoyun mentioned this pull request Jun 16, 2022
Open
@kakkoyun kakkoyun marked this pull request as draft June 16, 2022 13:54
@kakkoyun
Copy link
Member Author

@javierhonduco I'll wait for your input on this before I proceed with this.

@kakkoyun kakkoyun changed the title debuginfo: Improve metadata quality checking debuginfo: Improve debuginfo quality checking using metadata Jun 16, 2022
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from 24608eb to 9c9e038 Compare June 21, 2022 13:33
@kakkoyun kakkoyun marked this pull request as ready for review June 21, 2022 13:47
javierhonduco
javierhonduco reviewed Jun 21, 2022
View reviewed changes
Copy link
Contributor

@javierhonduco javierhonduco left a comment

Choose a reason for hiding this comment

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

Great stuff!! Loving this, and learned a bunch of Go-isms on the way!

Left a couple of nits and some questions, and will test this on my machine right now!

proto/parca/debuginfo/v1alpha1/debuginfo.proto Show resolved Hide resolved
ui/packages/shared/client/src/parca/debuginfo/v1alpha1/debuginfo.ts Show resolved Hide resolved
pkg/symbolizer/symbolizer.go Outdated Show resolved Hide resolved
pkg/symbol/elfutils/elfutils.go Show resolved Hide resolved
pkg/symbol/elfutils/elfutils.go Outdated Show resolved Hide resolved
pkg/debuginfo/store.go Outdated Show resolved Hide resolved
pkg/debuginfo/store.go Outdated Show resolved Hide resolved
pkg/debuginfo/store.go Outdated Show resolved Hide resolved
pkg/debuginfo/store.go Outdated Show resolved Hide resolved
pkg/debuginfo/store.go Show resolved Hide resolved
@kakkoyun kakkoyun marked this pull request as draft June 21, 2022 16:28
@kakkoyun kakkoyun added this to the v0.12.0 milestone Jun 28, 2022
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from 9c9e038 to e6edf49 Compare June 29, 2022 06:37
@kakkoyun kakkoyun mentioned this pull request Jun 29, 2022
Open
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from 1ddde9f to a5d7780 Compare June 29, 2022 09:39
@kakkoyun kakkoyun marked this pull request as ready for review June 29, 2022 09:40
@kakkoyun
Copy link
Member Author

golangci-lint fails because of golangci/golangci-lint#2859 I'll find a work around

And proto check failures should be fixed with #1260

javierhonduco
javierhonduco approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@javierhonduco javierhonduco left a comment

Choose a reason for hiding this comment

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

Looks great!!

Given that there are many changes here, could you run it for a little while on some test binary and see if everything works as expected?

🚢

pkg/debuginfo/store.go Outdated Show resolved Hide resolved
pkg/debuginfo/store.go Show resolved Hide resolved
pkg/symbol/elfutils/elfutils.go Show resolved Hide resolved
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from 82b4c6f to e51df4a Compare June 29, 2022 13:37
@kakkoyun kakkoyun marked this pull request as draft June 29, 2022 15:42
@kakkoyun
Copy link
Member Author

This is still being tested and blocked by #1265

kakkoyun added 2 commits June 30, 2022 13:38
@kakkoyun
Improve metadata quality checking
c2623ab 
Add hash to metadata file

Reduce metadata states

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix misusage of hash
af3f6fd 
Refactor manager

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
kakkoyun added 11 commits June 30, 2022 13:38
@kakkoyun
Refactor validity checks
c804815 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Reject if the received file is not valid
1c9c909 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Add test for invalid state
574145d 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Simplify exist call
258564d 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix CI issues
9ff1f1f 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Address review comments
1a3ce84 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Only buffer the header of the file to validate
6683f96 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Add debug lines
ef3e1c2 
Clean up

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Add limited readers
fd27a47 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Do not add flag servers not specified
c6d2384 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Add TODO items
69448eb 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from 87b540a to 0239e60 Compare June 30, 2022 11:39
@kakkoyun
Add build id to metadata file
8f5c156 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun force-pushed the add_hash_to_metadata branch from 0239e60 to 8f5c156 Compare June 30, 2022 12:39
@kakkoyun kakkoyun marked this pull request as ready for review June 30, 2022 12:40
@kakkoyun
Clean up
86a1d3a 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
javierhonduco
javierhonduco approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@javierhonduco javierhonduco left a comment

Choose a reason for hiding this comment

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

Amazing work!

brancz
brancz reviewed Jun 30, 2022
View reviewed changes
pkg/debuginfo/store.go Outdated Show resolved Hide resolved
@kakkoyun
Address review issues
a76da42 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun enabled auto-merge (squash) June 30, 2022 13:42
@kakkoyun
Only check ids when state is uploading
6bd20cb 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
brancz
brancz approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

Very nice! 💯

@kakkoyun kakkoyun disabled auto-merge June 30, 2022 14:31
@kakkoyun kakkoyun merged commit 911fda5 into parca-dev:main Jun 30, 2022
@kakkoyun kakkoyun deleted the add_hash_to_metadata branch June 30, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierhonduco javierhonduco javierhonduco approved these changes

@brancz brancz brancz approved these changes

@metalmatze metalmatze Awaiting requested review from metalmatze

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.

symbolizer: Reject invalid dwarf files
3 participants
@kakkoyun @brancz @javierhonduco