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.

Sign up for GitHub

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

Jump to bottom

chore: span origin #9012

Closed
wants to merge 15 commits into from
Closed

chore: span origin #9012

wants to merge 15 commits into from

Conversation

P403n1x87
Copy link
Contributor

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@P403n1x87 P403n1x87 self-assigned this Apr 17, 2024
@P403n1x87 P403n1x87 force-pushed the chore/span-origin branch 3 times, most recently from ebdadbc to 5a46550 Compare April 24, 2024 15:21
@P403n1x87 P403n1x87 added changelog/no-changelog A changelog entry is not required for this PR. Dynamic Instrumentation Dynamic Instrumentation/Live Debugger labels Apr 26, 2024
@P403n1x87 P403n1x87 force-pushed the chore/span-origin branch 2 times, most recently from 49ca44c to d0eca01 Compare May 22, 2024 09:29
@P403n1x87 P403n1x87 requested a review from shatzi May 29, 2024 13:24
shatzi
shatzi reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor

@shatzi shatzi 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

ddtrace/ext/__init__.py
@@ -22,3 +22,17 @@ class SpanKind(object):
SERVER = "server"
PRODUCER = "producer"
CONSUMER = "consumer"


EXIT_SPAN_TYPES = frozenset(
Copy link
Contributor

Choose a reason for hiding this comment

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

is that a complete list? what is the plan on how to maintain this list?

ddtrace/debugging/_origin/span.py
# the stack that belongs to user code.
# TODO: Add a limit to the number of frames to capture
seq = count(1)
for frame in frame_stack(sys._getframe(1)):
Copy link
Contributor

Choose a reason for hiding this comment

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

did we decided to add the entire stack for exit spans?
if so, then how do we deal the frame numbering? currently is running numbers of user-code frames. but if we also report the entire stack, maybe those should match the frame depth?

shatzi
shatzi approved these changes Jul 1, 2024
View reviewed changes
ddtrace/bootstrap/preload.py Show resolved Hide resolved
ddtrace/debugging/_origin/span.py Outdated

root = ddtrace.tracer.current_root_span()
location = self.location
if root is None or root.get_tag("_dd.entry_location.file") is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

java impl does something different, it add the tags to both the current active span and root span. not sure if we want to align that logic or not.

ddtrace/debugging/_origin/span.py Outdated
root.set_tag_str("_dd.entry_location.type", location.module)
root.set_tag_str("_dd.entry_location.method", location.name)

if config._trace_span_origin_enriched:
Copy link
Contributor

Choose a reason for hiding this comment

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

why we take snapshot at entry and not on exit? I think it better to snapshot exit with locals and return values?
do we capture locals at the moment or just return here?

Copy link
Contributor

Choose a reason for hiding this comment

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

second, I think we also need to look at _dd.p.debug flag for the enriched snapshots.

ddtrace/internal/telemetry/writer.py Outdated Show resolved Hide resolved
tests/debugging/origin/test_span.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the stale label Aug 31, 2024
@P403n1x87 P403n1x87 removed the stale label Sep 13, 2024
shatzi
shatzi approved these changes Sep 19, 2024
View reviewed changes
ddtrace/debugging/_origin/span.py Outdated
if is_user_code(frame_origin):
print(frame_origin, "is user code")
n = next(seq)
span.set_tag_str(f"_dd.ld.code_origin.frames.{n}.file", str(frame_origin.resolve()))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have method/type information here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, unless we query the garbage collector, which is expensive :(

@P403n1x87 P403n1x87 closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shatzi shatzi shatzi approved these changes

Assignees

@P403n1x87 P403n1x87

Labels
changelog/no-changelog A changelog entry is not required for this PR. Dynamic Instrumentation Dynamic Instrumentation/Live Debugger
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@P403n1x87 @shatzi