Skip to content

Commit

Permalink
fix(profiling): fix type check for exception types using isinstance […
Browse files Browse the repository at this point in the history
…backport 2.10] (#10013)

Backport f33856b from #9551 to 2.10.

Previously, `exc_type = <class Exception>` would have failed the if
check and now it passes as expected. This will result in showing
exception type names in Timeline view in profiling, which is gated by a
feature flag.

Tested using a sample application at
https://github.com/DataDog/profiling-timeline-python-sample-app
before:
<img width="291" alt="Screenshot 2024-06-18 at 5 32 14 PM"
src="https://github.com/DataDog/dd-trace-py/assets/6655247/72b8e897-9b1d-4238-a2d4-a77714493efd">

after: 
<img width="286" alt="Screenshot 2024-06-18 at 5 32 04 PM"
src="https://github.com/DataDog/dd-trace-py/assets/6655247/ff572098-d3d5-48cc-8993-6d9ba1b30a33">

Unfortunately, there's no easy way to write a Python unit test for this
change. As the change touches a Cython file which should be a thin layer
between a C++ extension and Python code. Also, we don't have any
existing tests written in Cython that can be run using pytest, which
could have been extended for this PR.

When DD_PROFILING_EXPORT_LIBDD_ENABLED is set and
DD_PROFILING_STACK_V2_ENABLED is not set, the following chain of calls
happens to collect exception information
- ddtrace.profiling.collector.stack.StackCollector.collect_stack
[link](https://github.com/DataDog/dd-trace-py/blob/16f3f951addfad0ba3b65235e601a9c101f273c6/ddtrace/profiling/collector/stack.pyx#L397)
-
ddtrace.internal.datadog.profiling.ddup.SampleHandle.push_exceptioninfo
[link](https://github.com/DataDog/dd-trace-py/blob/16f3f951addfad0ba3b65235e601a9c101f273c6/ddtrace/internal/datadog/profiling/ddup/_ddup.pyx#L257)
- ddup_push_exceptioninfo (extern C function)
[link](https://github.com/DataDog/dd-trace-py/blob/16f3f951addfad0ba3b65235e601a9c101f273c6/ddtrace/internal/datadog/profiling/dd_wrapper/src/interface.cpp#L238)
- Datadog::Sample::push_exceptioninfo (C++ function)
[link](https://github.com/DataDog/dd-trace-py/blob/16f3f951addfad0ba3b65235e601a9c101f273c6/ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp#L153)

The only way to inspect the collected samples is using
`Datadog::Sample::profile_borrow()` which isn't directly callable from a
Python unit test.

Also, when STACK_V2 is enabled, this whole code path can be removed so
I'd avoid spending too much time and effort in an interim solution,
though I already did.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: Taegyun Kim <[email protected]>
  • Loading branch information
github-actions[bot] and taegyunkim authored Aug 1, 2024
1 parent 9d8fdc6 commit dade0b0
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ddtrace/internal/datadog/profiling/ddup/_ddup.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ cdef class SampleHandle:
def push_exceptioninfo(self, exc_type: Union[None, bytes, str, type], count: int) -> None:
if self.ptr is not NULL:
exc_name = None
if exc_type is type:
if isinstance(exc_type, type):
exc_name = ensure_binary_or_empty(exc_type.__module__ + "." + exc_type.__name__)
else:
exc_name = ensure_binary_or_empty(exc_type)
Expand Down

0 comments on commit dade0b0

Please sign in to comment.