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

[Profiling .NET] Update the .NET profiling doc to support Single Step APM Instrumentation #27414

Merged
merged 14 commits into from
Feb 5, 2025

Conversation

chrisnas
Copy link
Contributor

@chrisnas chrisnas commented Feb 3, 2025

What does this PR do? What is the motivation?

We faced customer cases where the profiling documentation could be misleading when APM is deployed with Single Step Instrumentation. So, the enabling and troubleshooting parts of the .NET profiling documentation are updated to be more explicit.

Merge instructions

Merge readiness:

  • Ready for merge

Merge queue is enabled in this repo. To have it automatically merged after it receives the required reviews, create the PR (from a branch that follows the <yourname>/description naming convention) and then add the following PR comment:

/merge

Additional notes

@chrisnas chrisnas requested a review from a team as a code owner February 3, 2025 11:23
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Copy link
Member

@andrewlock andrewlock left a comment

Choose a reason for hiding this comment

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

LGTM overall, just some general questions/suggestions, thanks!

@buraizu
Copy link
Contributor

buraizu commented Feb 3, 2025

Hi team, just adding the work in progress label for now, as it appears there is some open feedback on the new content. Please feel free to remove the label when the content has been finalized and is ready for documentation team review.

@buraizu buraizu added the WORK IN PROGRESS No review needed, it's a wip ;) label Feb 3, 2025
@chrisnas chrisnas removed the WORK IN PROGRESS No review needed, it's a wip ;) label Feb 5, 2025
@chrisnas
Copy link
Contributor Author

chrisnas commented Feb 5, 2025

Hi team, just adding the work in progress label for now, as it appears there is some open feedback on the new content. Please feel free to remove the label when the content has been finalized and is ready for documentation team review.

@buraizu: It should be fine to merge now

Copy link
Contributor

@buraizu buraizu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, requesting some updates per the style guide and for clarity


1. Open the `dotnet-native-loader-dotnet-<pid>` log file in the `/var/log/datadog` folder.

2. Look for `CorProfiler::Initialize: Continuous Profiler initialized successfully.` near the end. If that message is not present, enable debug logs by setting the `DD_TRACE_DEBUG` environment variable for the application.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2. Look for `CorProfiler::Initialize: Continuous Profiler initialized successfully.` near the end. If that message is not present, enable debug logs by setting the `DD_TRACE_DEBUG` environment variable for the application.
2. Look for `CorProfiler::Initialize: Continuous Profiler initialized successfully` near the end. If that message is not present, enable debug logs by setting the `DD_TRACE_DEBUG` environment variable for the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a string that appears in the log file so I would not remove the .

Comment on lines +135 to +136
- `Profiler signal handler was replaced again. It will not be restored: the profiler is disabled.`
- `Fail to restore profiler signal handler.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `Profiler signal handler was replaced again. It will not be restored: the profiler is disabled.`
- `Fail to restore profiler signal handler.`
- `Profiler signal handler was replaced again. It will not be restored: the profiler is disabled`
- `Fail to restore profiler signal handler`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these strings appear as is in the log file, the . should not be removed.

@buraizu
Copy link
Contributor

buraizu commented Feb 5, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Feb 5, 2025

Devflow running: /merge

View all feedbacks in Devflow UI.


2025-02-05 19:26:21 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in master is 7m.


2025-02-05 19:36:17 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit d7ea691 into master Feb 5, 2025
18 of 20 checks passed
@dd-mergequeue dd-mergequeue bot deleted the chrisnas/dotnet_update_for_ssi branch February 5, 2025 19: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.

5 participants