[PROF-10028] Avoid re-uploading already successfully uploaded file #24
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gandem
force-pushed
the
nayef/upload-cache
branch
2 times, most recently
from
3d86a02 to
04afab1
Compare
r1viollet
reviewed
Jun 25, 2024
| return &DatadogUploader{ | ||
| ddAPIKey: ddAPIKey, | ||
| intakeURL: intakeURL, | ||
|
|
There was a problem hiding this comment.
minor: why the extra lines (mostly curious)
There was a problem hiding this comment.
mostly cosmetic to separate fields that are based on user input, from internal fields that are initialized by the structure itself
r1viollet
reviewed
Jun 25, 2024
| const sourceMapEndpoint = "/api/v2/srcmap" | ||
|
|
||
| type DatadogUploader struct { | ||
| ddAPIKey string | ||
| intakeURL string | ||
|
|
||
| uploadCache *lru.SyncedLRU[libpf.FileID, struct{}] |
There was a problem hiding this comment.
the choice of file ID seems good
If we want to consider split debug, we would probably not cache the id of the split file, only of the main binary.
There was a problem hiding this comment.
Agreed 👍 That's how it's implemented in #25 AFAICT 👌
r1viollet
previously approved these changes
Jun 25, 2024
nsavoire
previously approved these changes
Jun 25, 2024
Gandem
dismissed stale reviews from nsavoire and r1viollet
The base branch was changed.
Gandem
force-pushed
the
nayef/upload-cache
branch
from
04afab1 to
1e4ada9
Compare
nsavoire
approved these changes
Jun 25, 2024
Gandem
added a commit
that referenced
this pull request
Jun 26, 2024
Gandem
added a commit
that referenced
this pull request
Jun 26, 2024
Gandem
added a commit
that referenced
this pull request
Jun 27, 2024
Gandem
added a commit
that referenced
this pull request
Jun 28, 2024
Gandem
added a commit
that referenced
this pull request
Jun 28, 2024
Gandem
added a commit
that referenced
this pull request
Jun 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking at a local run of #23 I noticed the following logs (truncated), which come from uploading the local symbols for /usr/bin/docker multiple times.
This happens because the process manager only keeps track of executables as long as they are mapped for an existing process. In the case above, a process is running the
dockerCLI, so the process manager sees the executable a first time. Then thedockerCLI commands exists, the process manager forgets about that executable (throughRemoveOrDecRef()). The next time adockerCLI process is launched, the process manager will consider it as a new executable, and hence we try to re-upload symbols for that binary.In the short-term, we can work around this by adding a cache on already uploaded files and avoid re-uploads if they were previously already uploaded successfully.