-
Notifications
You must be signed in to change notification settings - Fork 424
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: integration with dd-trace-api #12057
base: main
Are you sure you want to change the base?
Conversation
|
BenchmarksBenchmark execution time: 2025-02-05 17:30:28 Comparing candidate commit 0e679f8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 394 metrics, 2 unstable metrics. |
Datadog ReportBranch report: ✅ 0 Failed, 130 Passed, 1468 Skipped, 4m 38.63s Total duration (35m 46.84s time saved) |
def test_set_exc_info(self): | ||
with dd_trace_api.tracer.trace("web.request") as span: | ||
try: | ||
raise Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ Code Quality Violation
Exception is too generic (...read more)
Do not raise Exception
and BaseException
. These are too generic. Having generic exceptions makes it difficult to differentiate errors in a program. Use a specific exception, for example, ValueError
, or create your own instead of using generic ones.
Learn More
def test_set_traceback(self): | ||
with dd_trace_api.tracer.trace("web.request") as span: | ||
try: | ||
raise Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ Code Quality Violation
Exception is too generic (...read more)
Do not raise Exception
and BaseException
. These are too generic. Having generic exceptions makes it difficult to differentiate errors in a program. Use a specific exception, for example, ValueError
, or create your own instead of using generic ones.
Learn More
return getattr(dd_trace_api, "__version__", "") | ||
|
||
|
||
def patch(tracer=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the integration patch/unpatch approach makes sense here, should we always register an audit hook that listens for these events?
are there ever any cases when we wouldn't want to listen to the events? it would probably be better to have dd_trace_api
not emit events if you want to "disable" vs having that handled here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patch
, unpatch
and get_version
are here just to make this code work with the existing contrib structure. The contrib is enabled by default, and it can be disabled by the same method as any other default-on contrib. As far as I can tell, this code makes sense as a contrib because that provides lazy loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the lazy loading makes sense, but I am not sure we should provide all the same functionality, like DD_DD_TRACE_API_ENABLED=false
or DD_PATCH_MODULES=dd_trace_api:false
or patch_all(dd_trace_api=False)
, etc (or w/e all those options are).
lazy loading, reporting usage and the version to telemetry both definitely make sense.
return getattr(dd_trace_api, "__version__", "") | ||
|
||
|
||
def patch(tracer=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the lazy loading makes sense, but I am not sure we should provide all the same functionality, like DD_DD_TRACE_API_ENABLED=false
or DD_PATCH_MODULES=dd_trace_api:false
or patch_all(dd_trace_api=False)
, etc (or w/e all those options are).
lazy loading, reporting usage and the version to telemetry both definitely make sense.
This change adds an integration for the dd-trace-api-py package. That package is still in pre-release development, so the code this change adds will not be exercised except in CI.
The basic integration added here instruments the interface exposed by the dd-trace-api package, proxying its calls to real
Tracer
andSpan
objects. The basic functionality of tracing is implemented, includingstart_span
,trace
,finish
, andcurrent_span
. More operations, like setting links or tags on spans, will be implemented in a future pull request. Other parts of the public API likePin
anddata_streams
will also be included in future pull requests.Checklist
Reviewer Checklist