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

Point to latest version of our otel profiler fork #2973

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

umanwizard
Copy link
Contributor

@umanwizard umanwizard commented Aug 28, 2024

There are several innocuous changes, but please take a look at the comment beginning !! FIXME !!. We will have to address that soon, I think.

@umanwizard umanwizard requested a review from brancz August 28, 2024 19:58
@umanwizard umanwizard marked this pull request as draft August 28, 2024 20:44
@umanwizard
Copy link
Contributor Author

Don't merge this yet -- it's not uploading any profiles when I run it locally; something must be wrong.

You now need to call `StartRealtimeSync` on startup, or the reported
timestamps will be relative to system boot time, rather than to the
Unix epoch.
@umanwizard
Copy link
Contributor Author

Now ready for a look.

@umanwizard umanwizard marked this pull request as ready for review August 29, 2024 01:43
@@ -244,7 +241,9 @@ func (r *ParcaReporter) ExecutableMetadata(ctx context.Context,
}

// Always attempt to upload, the uploader is responsible for deduplication.
r.uploader.Upload(ctx, fileID, buildID, open)
// !! FIXME !! Apparently uploading stuff in this function is a "really bad idea";
// see https://github.com/open-telemetry/opentelemetry-ebpf-profiler/pull/126#pullrequestreview-2248554710
Copy link
Member

Choose a reason for hiding this comment

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

The recommendation is to specifically not block this Go routine, but instead open the file and pass the file handle to an asynchronous mechanism. Apart from immediately opening the file, that's exactly how our mechanism works, so we can remove the context here without a problem.

reporter/parca_reporter.go Outdated Show resolved Hide resolved
@umanwizard umanwizard merged commit b27974c into parca-dev:main Sep 3, 2024
3 checks passed
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.

2 participants