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

profiler: add tag to indicate whether execution tracing is enabled #1997

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

nsrip-dd
Copy link
Contributor

@nsrip-dd nsrip-dd commented May 23, 2023

What does this PR do?

Adds a go_execution_trace_enabled tag to uploaded profiles.

Motivation

Indicate whether execution tracing is enabled, to distinguish between missing a
trace because we don't collect them every profiling cycle from missing a trace
because the feature isn't turned on.

While we're here, add these extra tracing tags to the batch object
that's built during the profiling cycle. We can't safely read the
execution trace config during the uploading, because it can be modified
concurrently by the profiler after #1978. It makes more sense to have
the profiler add all the needed information, which it can do safely, so
that the upload step can just deal with uploading.

Describe how to test/QA your changes

This PR includes a unit test.

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@pr-commenter
Copy link

pr-commenter bot commented May 23, 2023

Benchmarks

Benchmark execution time: 2023-06-09 13:03:58

Comparing candidate commit 62f2cc8 in PR branch nick.ripley/add-exec-tracing-enabled-tag with baseline commit 8c2d9e1 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 0 unstable metrics.

@nsrip-dd nsrip-dd force-pushed the nick.ripley/add-exec-tracing-enabled-tag branch 2 times, most recently from 73530eb to 05717e3 Compare May 23, 2023 18:55
go_execution_trace_enabled indicates whether execution tracing is
enabled, to distinguish between missing a trace because we don't collect
them every profiling cycle from missing a trace because the feature
isn't turned on.

While we're here, add these extra tracing tags to the batch object
that's built during the profiling cycle. We can't safely read the
execution trace config during the uploading, because it can be modified
concurrently by the profiler after #1978. It makes more sense to have
the profiler add all the needed information, which it can do safely, so
that the upload step can just deal with uploading.
@nsrip-dd nsrip-dd force-pushed the nick.ripley/add-exec-tracing-enabled-tag branch from 05717e3 to f058aed Compare May 24, 2023 12:47
@nsrip-dd nsrip-dd marked this pull request as ready for review June 7, 2023 17:33
@nsrip-dd nsrip-dd requested a review from a team as a code owner June 7, 2023 17:33
@nsrip-dd nsrip-dd marked this pull request as draft June 8, 2023 12:43
@nsrip-dd
Copy link
Contributor Author

nsrip-dd commented Jun 8, 2023

Moved back to a draft, I might change this to put the indicator in the event.json rather than as a tag per Ivo's internal RFC. Will resolve soon. NVM, tag will be easier 👍

@nsrip-dd nsrip-dd marked this pull request as ready for review June 8, 2023 19:56
felixge
felixge previously approved these changes Jun 8, 2023
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

Code LGTM. But let's try to discuss the question I added.

profiler/profiler.go Outdated Show resolved Hide resolved
To make non-user-facing tags like this easier to hide in the future.
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

LGTM for the next release. That being said, I think we should do some more work on internal alignment on how we want to handle this consistently across client libs. Ivo's proposal was different.

@nsrip-dd nsrip-dd merged commit f3aa42a into main Jun 9, 2023
@nsrip-dd nsrip-dd deleted the nick.ripley/add-exec-tracing-enabled-tag branch June 9, 2023 13:24
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