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

feat(profiler) add exception message to exception sample #2434

Merged
merged 8 commits into from
Feb 19, 2024

Conversation

realFlowControl
Copy link
Collaborator

@realFlowControl realFlowControl commented Dec 22, 2023

Description

This will add the exception message to the sample that is taken. You'd need to opt-in to the collection of exception messages by setting the environment variable DD_PROFILING_EXCEPTION_MESSAGE_ENABLED or the INI setting datadog.profiling.exception_message_enabled to true/1/on.

Additionally I moved reading the exception class name (and the message) to after the sampling decision which lowers the overhead of exception profiling.

PROF-8838

Reviewer checklist

  • Backend and UI support is implemented
  • Test coverage seems ok.
  • Appropriate labels assigned.

@realFlowControl realFlowControl requested review from a team as code owners December 22, 2023 13:53
@github-actions github-actions bot added the profiling Relates to the Continuous Profiler label Dec 22, 2023
@realFlowControl realFlowControl marked this pull request as draft December 22, 2023 13:55
@realFlowControl realFlowControl self-assigned this Dec 22, 2023
@pr-commenter
Copy link

pr-commenter bot commented Dec 22, 2023

Benchmarks

Benchmark execution time: 2024-02-19 11:14:01

Comparing candidate commit 486661c in PR branch florian/exception-message with baseline commit 7da29d0 in branch master.

Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥
Check Gitlab CI job log to find if any benchmark has crashed.

Scenarios present only in baseline:

  • BM_TeaSapiSpindown
  • BM_TeaSapiSpinup

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

profiling/build.rs Outdated Show resolved Hide resolved
profiling/src/exception.rs Outdated Show resolved Hide resolved
profiling/src/php_ffi.h Show resolved Hide resolved
profiling/src/profiling/mod.rs Outdated Show resolved Hide resolved
@realFlowControl realFlowControl marked this pull request as ready for review January 8, 2024 10:42
Copy link
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

Are there any concerns about gathering the exception message? Any filtering or anything that needs done? I wonder if APM does anything.

@realFlowControl
Copy link
Collaborator Author

@morrisonlevi I added an option to disable the collection of messages for exceptions. It is default enabled

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

Merging #2434 (486661c) into master (7da29d0) will decrease coverage by 0.02%.
The diff coverage is 25.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2434      +/-   ##
============================================
- Coverage     76.67%   76.65%   -0.02%     
  Complexity      267      267              
============================================
  Files           136      136              
  Lines         17627    17629       +2     
  Branches       1034     1034              
============================================
- Hits          13515    13514       -1     
- Misses         3575     3578       +3     
  Partials        537      537              
Flag Coverage Δ
appsec-extension 70.24% <ø> (ø)
tracer-extension 78.61% <25.00%> (-0.03%) ⬇️
tracer-integrations 79.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
zend_abstract_interface/symbols/lookup.c 82.58% <25.00%> (-1.34%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da29d0...486661c. Read the comment docs.

@realFlowControl
Copy link
Collaborator Author

@morrisonlevi I am btw. open to set this default disabled

Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Looks good to me now :-)

@realFlowControl realFlowControl merged commit 60100d9 into master Feb 19, 2024
619 of 624 checks passed
@realFlowControl realFlowControl deleted the florian/exception-message branch February 19, 2024 11:42
@github-actions github-actions bot added this to the 0.98.0 milestone Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Relates to the Continuous Profiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants