Skip to content

GH-98894: Fix function__return and function__entry dTrace probe missing after GH-103083 #125019

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Oct 6, 2024

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 6, 2024

After #103083, the function__return and function__entry dTrace probe is missing. In the production environment, the developer may combine the dtrace probe with other tools like eBPF to trace the internal state of the CPython like

sudo bpftrace -e '                                                                         
usdt:/home/manjusaka/Documents/projects/cpython/python:python:function__return {
printf("filename: %s, funcname:%s, lineno:%d\n",str(arg0),str(arg1),arg2);
}
' -p 291832

And the function__return and function__entry part is still in the official document of the CPython for the Dtrace module.

So I think we should keep the codebase same with the document or we need to update the document if we confirm that we don't need the dtrace feature any more.

@picnixz
Copy link
Member

picnixz commented Oct 6, 2024

The build errors:

/tmp/tmpwuomaa14/_RETURN_VALUE.c:124:13: error: call to undeclared function 'PyDTrace_FUNCTION_RETURN_ENABLED'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
124 | DTRACE_FUNCTION_EXIT();

Something is probably missing somewhere.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 6, 2024

The build errors:

/tmp/tmpwuomaa14/_RETURN_VALUE.c:124:13: error: call to undeclared function 'PyDTrace_FUNCTION_RETURN_ENABLED'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
124 | DTRACE_FUNCTION_EXIT();

Something is probably missing somewhere.

I have noticed this, I'm working on the CI. Thanks for the tips. Draft this PR first

@Zheaoli Zheaoli marked this pull request as draft October 6, 2024 11:20
@Zheaoli Zheaoli marked this pull request as ready for review October 6, 2024 14:20
@Zheaoli Zheaoli requested a review from brandtbucher as a code owner October 6, 2024 14:20
@Zheaoli Zheaoli force-pushed the manjusaka/fix-dtrace branch from 87ac00a to 128e856 Compare October 7, 2024 15:26
@markshannon
Copy link
Member

DTRACE_FUNCTION_ENTRY() and DTRACE_FUNCTION_EXIT() are static instrumentation points for dtrace, are they not?

IIUC, this means that they will not work with the JIT. Even if the instrumentation points are compiled into the templates, dtrace will not be able to find them.

How does dtrace handle jit-in-time compiled code?

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 9, 2024

DTRACE_FUNCTION_ENTRY() and DTRACE_FUNCTION_EXIT() are static instrumentation points for dtrace, are they not?

Yes, it's static

IIUC, this means that they will not work with the JIT. Even if the instrumentation points are compiled into the templates, dtrace will not be able to find them.

How does dtrace handle jit-in-time compiled code?

The DTRACE_FUNCTION_ENTRY() and DTRACE_FUNCTION_EXIT() is not working in JIT mode. For now I just compile some empty function into the template to avoid the compile issue.

For now, the JIT is still an experimental feature. I think DTRACE_FUNCTION_ENTRY() and DTRACE_FUNCTION_EXIT() are still useful for normal code.

For the future, I think we may need extra dtrace point for JIT module

@markshannon
Copy link
Member

For the future, I think we may need extra dtrace point for JIT module

How?
There's no point in adding back dtrace support for 3.14, if it stops working again in 3.15.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 10, 2024

How?

That would be some different dtrace points, I'm not sure we need to discuss it here.

There's no point in adding back dtrace support for 3.14, if it stops working again in 3.15.

I'm not sure about the JIT roadmap. if here's more than five years before we make the JIT default release, I think it still is worthed adding the dtrace point back.

Otherwise, we need to clean up the docs FYI https://docs.python.org/3/howto/instrumentation.html

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 14, 2024

@markshannon ping~

@thesamesam
Copy link
Contributor

Also, merging it as it is facilitates backporting.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 22, 2024

@markshannon ping~

@markshannon
Copy link
Member

I'm not sure about the JIT roadmap. if here's more than five years before we make the JIT default release, I think it still is worthed adding the dtrace point back.

The JIT will be included in 3.14, but probably off by default. It will almost certainly on by default for 3.15.

@markshannon
Copy link
Member

I'm not opposed to having dtrace hooks, but I don't see much value in them unless they

  • have tests (which means they built in by default, so CI can run the tests)
  • will work with the JIT
  • have very low overhead when not in use (should be true for the interpreter, but might not be for the JIT)

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 23, 2024

I'm not opposed to having dtrace hooks, but I don't see much value in them unless they

  • have tests (which means they built in by default, so CI can run the tests)
  • will work with the JIT
  • have very low overhead when not in use (should be true for the interpreter, but might not be for the JIT)

OK, I got it. 1 and 3 would not be a big issue, but I need more time about 2. So how about we update https://docs.python.org/3/howto/instrumentation.html and remove the function__return and function__entry part first?

@x-zvf
Copy link

x-zvf commented Mar 29, 2025

+1
(bpf|d)trace instrumentation is an important feature for us. Is someone still working on it? If not, please add a note in the documentation that USDT probes are broken after python3.10, and users should avoid upgrading.

@furkanonder
Copy link
Contributor

@Zheaoli Can you resolve the conflicts?

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Apr 4, 2025

@Zheaoli Can you resolve the conflicts?

Yes, But I think we need a final call here.

@markshannon Should we recover the USDT probe or we just need to remove it from documentation?

@ajor
Copy link

ajor commented Apr 23, 2025

If the Python JIT can be enabled/disabled without a recompilation of Python, then to me it seems worth it to include this instrumentation for non-JIT codepaths at least.

That way, people who want instrumentation can disable the JIT to get it as needed. It can be documented that the probe points won't work when the JIT is enabled (rather than the alternative of them not working at all!)

@x-zvf
Copy link

x-zvf commented Apr 23, 2025

If the Python JIT can be enabled/disabled without a recompilation of Python, then to me it seems worth it to include this instrumentation for non-JIT codepaths at least.

That way, people who want instrumentation can disable the JIT to get it as needed. It can be documented that the probe points won't work when the JIT is enabled (rather than the alternative of them not working at all!)

This seems like the easiest path to move forward with, but it defeats the greatest power of Dtrace - intrumenting running processes (with low overhead). If you need to restart a process to instrument it (disabling the jit), the probes become much less useful.
That said, this is still much preferable to removing the probes :)

@thesamesam
Copy link
Contributor

thesamesam commented Apr 23, 2025

I'm also confident that I can get eyes on the general DTrace vs JIT issue in the future, just at the moment, the team working on that is busy with some more fundamental bits on the DTrace side. That is, I don't think this is delaying the inevitable or anything (and therefore PR should ideally go in for now).

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.

Some DTrace probes are broken in 3.11
8 participants