Add simple method tracing API#5294
Conversation
|
👋 Hey @lloeki, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2026-02-03 11:52:41 UTC |
9e88d86 to
e51e2d4
Compare
BenchmarksBenchmark execution time: 2026-02-06 10:42:46 Comparing candidate commit 47df115 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:tracing - Propagation - Datadog
|
4ff2738 to
e3b58b9
Compare
|
Strange CI failures, apparently unrelated, like: and summaries: |
3ec50d5 to
cdafd12
Compare
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 47df115 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
|
Steep hits a: EDIT: soutaro/steep#2015 |
Strech
left a comment
Strech
left a comment
There was a problem hiding this comment.
I think it looks simple and effective 👏🏼
Some ideas:
- Improve slightly documentation in the comments (like YARD for arguments)
- Maybe some areas of wrong-usage should be covered in tests
- If possible avoid using
eval, but only if it doesn't make it much more complicated
P.S it's a non-blocking suggestions
|
I support |
@p-datadog I agree this is interesting to support. The reason why it wasn't is that the initial use case described was this DSL: A corresponding usage with Since these methods are dynamic (virtual? is that a better term?), they "exist" only at runtime, and can only be checked for via Therefore they can't be checked for existence at the class level, which is where the DSL operates. With the current implementation that enforces the check, both I am considering an implementation where the user can simply relax that existence (+visibility) check, since the check simply can't be done at the class level. Visibility doesn't matter anyway as With that check relaxed, both A side effect is that a An alternative considered was to hook on So... the other reason why is that it's more complex to handle and I focused on laying down the basics ;) |
|
Dug into steep with: $ find lib -name '*.rb' | while read -r f; do echo -n "#### $f"; out=$(steep check "$f" 2>&1); rc=$?; if [[ $rc -ne 0 || "$out" == *FATAL* ]]; then echo -e "\rFAIL $f"; echo "$f" >> steep.failed; else echo -e "\r OK $f"; fi; done
-----8<----
$ cat steep.failed
lib/datadog/kit/appsec/events/v2.rb
lib/datadog/tracing/contrib/rack/route_inference.rb
lib/datadog/core/logger.rbSteep failures: $ steep check lib/datadog/core/logger.rb; echo "=> exit: $?"
2026-02-04 15:29:23.361: FATAL: [Steep 1.10.0] [typecheck:typecheck@6] [background] [#typecheck_source(path=lib/datadog/core/logger.rb)] [#type_check_file(lib/datadog/core/logger.rb@datadog)] [synthesize:(3:1)] [synthesize:(5:1)] [synthesize:(6:3)] [synthesize:(11:5)] [synthesize:(12:7)] [synthesize:(20:7)] [synthesize:(21:9)] [synthesize:(29:9)] [synthesize:(30:11)] [synthesize:(31:13)]
Unexpected error: #<NoMethodError: undefined method 'selector' for an instance of Parser::Source::Map::Keyword>
-----8<----
=> exit: 0$ steep check lib/datadog/tracing/contrib/rack/route_inference.rb; echo "=> exit: $?"
2026-02-04 15:27:49.863: FATAL: [Steep 1.10.0] [typecheck:typecheck@6] [background] [#typecheck_source(path=lib/datadog/tracing/contrib/rack/route_inference.rb)] [#type_check_file(lib/datadog/tracing/contrib/rack/route_inference.rb@datadog)] [synthesize:(3:1)] [synthesize:(4:3)] [synthesize:(5:5)] [synthesize:(6:7)] [synthesize:(9:9)] [synthesize:(10:11)] [synthesize:(27:11)] [synthesize:(28:13)] [synthesize:(28:13)] [synthesize:(30:13)]
Unexpected error: #<ArgumentError: wrong number of arguments (given 0, expected 3)>
-----8<----
=> exit: 0$ steep check lib/datadog/kit/appsec/events/v2.rb; echo "=> exit: $?"
2026-02-04 15:31:18.321: FATAL: [Steep 1.10.0] [typecheck:typecheck@7] [background] [#typecheck_source(path=lib/datadog/kit/appsec/events/v2.rb)] [#type_check_file(lib/datadog/kit/appsec/events/v2.rb@datadog)] [synthesize:(3:1)] [synthesize:(6:1)] [synthesize:(7:3)] [synthesize:(8:5)] [synthesize:(9:7)] [synthesize:(11:9)] [synthesize:(12:11)] [synthesize:(22:11)] [synthesize:(44:13)] [synthesize:(44:13)] [synthesize:(45:15)] [synthesize:(48:15)] [synthesize:(48:18)] [synthesize:(48:18)]
Unexpected error: #<RuntimeError: Unknown name for build_instance: Tracing::TraceSegment>
-----8<----
=> exit: 1The latter has these type failures: |
Typing analysisIgnored filesThis PR introduces 4 ignored files. It decreases the percentage of typed files from 42.11% to 41.73% (-0.38%). Ignored files (+4-0)❌ Introduced:Note: Ignored files are excluded from the next sections. Untyped methodsThis PR clears 2 untyped methods and 4 partially typed methods. It increases the percentage of typed methods from 59.57% to 59.65% (+0.08%). Untyped methods (+0-2)✅ Cleared:Partially typed methods (+0-4)✅ Cleared:Untyped other declarationsThis PR clears 1 untyped other declaration and 2 partially typed other declarations. It decreases the percentage of typed other declarations from 76.42% to 76.22% (-0.2%). Untyped other declarations (+0-1)✅ Cleared:Partially typed other declarations (+0-2)✅ Cleared: |
Traced down to this line
So the problem is Lots of diff --git a/lib/datadog/core/logger.rb b/lib/datadog/core/logger.rb
index c551660f98..14b2b704ad 100644
--- a/lib/datadog/core/logger.rb
+++ b/lib/datadog/core/logger.rb
@@ -28,7 +28,7 @@ module Datadog
if message.nil?
if block
- super do
+ super(severity) do
"[#{self.progname}] #{where}#{yield}"
end
elseWill report details properly upstream. |
658bf11 to
71268e6
Compare
mabdinur
left a comment
mabdinur
left a comment
There was a problem hiding this comment.
left some questions, overall LGTM thanks for picking this up 😄
p-datadog
left a comment
p-datadog
left a comment
There was a problem hiding this comment.
I approve based on my previous comments.
| # | ||
| # def foo; 'hello'; end | ||
| # | ||
| # trace_method :foo, span_name: 'optional_span_name' |
There was a problem hiding this comment.
It would be nice to also see an example of how to add tracing to a class method.
There was a problem hiding this comment.
Fair point.
Given that:
- "class methods" don't exist! They are merely methods of instances of the class
Class - but we wouldn't want a given method to be hooked for every single class!
... one would straight up hook onto that instance's singleton, i.e MyClass.singleton_class:
class MyClass
class << self
def foo; 'hello'; end
end
end
Datadog::Kit::Tracing::MethodTracer.trace_method(MyClass.singleton_class, :foo, span_name: 'MyClass.foo')Alternatively, this works too:
class MyClass
class << self
extend Datadog::Kit::Tracing::MethodTracer
def foo; 'hello'; end
trace_method :foo, span_name: 'MyClass.foo'
end
endSince this singleton class is a dynamic class, span_name must be provided.
A convenience DSL-ish class-context trace_class_method (or maybe better: trace_singleton_class_method could be added:
def trace_singleton_class_method(method_name, span_name: nil, dynamic: false)
span_name ||= "#{name}.#{method_name}" if respond_to?(:name)
Datadog::Kit::Tracing::MethodTracer.trace_method(self.singleton_class, :foo, span_name: span_name, dynamic: false)I will open opened an issue to track that.
There was a problem hiding this comment.
A convenience DSL-ish class-context
trace_class_method(or maybe better:trace_singleton_class_methodcould be added
👍🏼 Yes, I was looking for something along those lines. (It also matches the API that we suggested way back in #670).
7 participants
What does this PR do?
Add a simple method tracing API
Motivation:
A long overdue and oft-requested feature #1197
Change log entry
Add simple method tracing API
Additional Notes:
This uses GraftThe intent is to bootstrap usage and kickstart implementation of an instrumentation APIRelated existing work:
DD_TRACE_METHODS(.Net, PHP, Java), system properties (Java): this needs to transform a (list of)Stringinto a constant + method; such an operation is carried out by Graft (HookPoint.parse,HookPoint.resolve_module); the constants and methods can only be resolved if they are defined before configuration happens; this significantly increases complexity and is out of scope for this PR.Things that these language define or allow setting:
name), Node (asname), PHP (asname))How to test the change?