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: record Orchestrion, activation information #2814

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

nsrip-dd
Copy link
Contributor

@nsrip-dd nsrip-dd commented Aug 8, 2024

What does this PR do?

For tracking adoption of SSI for profiling, record whether the profiler
was added using Orchestrion and how the profiler was activated,
following the internal "Adding SSI information to profiles" RFC.

The tests aren't great for this. I'm working on a more end-to-end test which
builds an app with Orchestrion, enables the profiler, and checks the contents of
the profile, possibly using our profiling-backend-in-a-box. In the mean time,
I ran the new test with Orchestrion to confirm that the "mechanism" is set:

% ~/repos/orchestrion/orchestrion go test -run=TestOrchestrionProfileInfo -v
=== RUN   TestOrchestrionProfileInfo
=== RUN   TestOrchestrionProfileInfo/env=""
    profiler_test.go:768: {SSI:{Mechanism:orchestrion} Activation:manual}
=== RUN   TestOrchestrionProfileInfo/env="1"
    profiler_test.go:768: {SSI:{Mechanism:orchestrion} Activation:manual}
=== RUN   TestOrchestrionProfileInfo/env="true"
    profiler_test.go:768: {SSI:{Mechanism:orchestrion} Activation:manual}
=== RUN   TestOrchestrionProfileInfo/env="auto"
    profiler_test.go:768: {SSI:{Mechanism:orchestrion} Activation:auto}
--- PASS: TestOrchestrionProfileInfo (0.11s)
    --- PASS: TestOrchestrionProfileInfo/env="" (0.04s)
    --- PASS: TestOrchestrionProfileInfo/env="1" (0.02s)
    --- PASS: TestOrchestrionProfileInfo/env="true" (0.02s)
    --- PASS: TestOrchestrionProfileInfo/env="auto" (0.02s)
PASS
ok  	gopkg.in/DataDog/dd-trace-go.v1/profiler	0.448s

Motivation

To track Orchestrion adoption in the same way that we'll track SSI adoption for
other languages/runtimes.

@pr-commenter
Copy link

pr-commenter bot commented Aug 8, 2024

Benchmarks

Benchmark execution time: 2024-08-30 19:22:23

Comparing candidate commit b70735a in PR branch PROF-10258-add-injection-info with baseline commit e7ca17e in branch main.

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

@nsrip-dd nsrip-dd force-pushed the PROF-10258-add-injection-info branch from 614baad to 97e641d Compare August 8, 2024 18:40
Copy link

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Stuck for more than 1 month label Aug 29, 2024
@nsrip-dd nsrip-dd force-pushed the PROF-10258-add-injection-info branch from 97e641d to 0e0f118 Compare August 30, 2024 18:53
@nsrip-dd nsrip-dd removed the stale Stuck for more than 1 month label Aug 30, 2024
For tracking adoption of SSI for profiling, record whether the profiler
was added using Orchestrion and how the profiler was activated,
following the internal "Adding SSI information to profiles" RFC.
@nsrip-dd nsrip-dd force-pushed the PROF-10258-add-injection-info branch from 0e0f118 to b70735a Compare August 30, 2024 18:56
@nsrip-dd nsrip-dd marked this pull request as ready for review August 30, 2024 18:57
@nsrip-dd nsrip-dd requested a review from a team as a code owner August 30, 2024 18:57
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 🙇

@nsrip-dd nsrip-dd enabled auto-merge (squash) September 10, 2024 14:31
@nsrip-dd nsrip-dd merged commit 0ffa615 into main Sep 10, 2024
134 of 135 checks passed
@nsrip-dd nsrip-dd deleted the PROF-10258-add-injection-info branch September 10, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants