Skip to content

Add ELF symbol upload#3

Merged
nsavoire merged 5 commits into
mainfrom
nsavoire/symbol_upload
Sep 19, 2024
Merged

Add ELF symbol upload#3
nsavoire merged 5 commits into
mainfrom
nsavoire/symbol_upload

Conversation

@nsavoire
Copy link
Copy Markdown
Collaborator

@nsavoire nsavoire commented Sep 6, 2024

@nsavoire nsavoire requested a review from a team as a code owner September 6, 2024 14:36
@nsavoire nsavoire force-pushed the nsavoire/symbol_upload branch from ecd0264 to 4fe01fb Compare September 6, 2024 15:08
@nsavoire nsavoire force-pushed the nsavoire/symbol_upload branch from 4fe01fb to d40603c Compare September 9, 2024 09:45
Copy link
Copy Markdown
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the port.

Copy link
Copy Markdown
Member

@Gandem Gandem left a comment

Choose a reason for hiding this comment

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

Made a first pass, will continue reviewing in a few 🙇

Comment thread cli_flags.go Outdated
Comment thread reporter/symbol_uploader.go Outdated
Copy link
Copy Markdown
Member

@Gandem Gandem left a comment

Choose a reason for hiding this comment

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

Made a second pass, thanks 🙇

Comment thread reporter/symbol_uploader.go Outdated
Comment thread reporter/symbol_uploader.go
Comment thread reporter/symbol_uploader.go Outdated
Comment thread reporter/symbol_uploader.go
Comment thread reporter/symbol_uploader.go Outdated
Comment thread reporter/symbol_uploader.go Outdated
Comment thread reporter/symbol_uploader.go Outdated
@nsavoire nsavoire force-pushed the nsavoire/symbol_upload branch from 40fd52f to 1a65e49 Compare September 18, 2024 23:03
Comment thread reporter/datadog_reporter.go
)

const uploadCacheSize = 16384
const uploadQueueSize = 1000
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.

I checked one of the most loaded relenv nodes, and on startup we seeing around 200 symbols uploaded, so the order of magnitude for the upload queue size LGTM 👍

Comment thread reporter/symbol_uploader.go Outdated
Comment thread reporter/symbol_uploader.go
@nsavoire nsavoire requested a review from Gandem September 19, 2024 09:10
Copy link
Copy Markdown
Member

@Gandem Gandem left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Comment thread reporter/symbol_uploader.go Outdated
Co-authored-by: Nayef Ghattas <nayef.ghattas@gmail.com>
@nsavoire nsavoire merged commit 3894eaa into main Sep 19, 2024
@nsavoire nsavoire deleted the nsavoire/symbol_upload branch September 19, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants