Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
feat: integration with dd-trace-api #12057
Changes from all commits
eb66781
198aaa9
d60b73c
4e1004d
dae3efd
3e40d48
181b1c1
ffc79e5
89e04a1
9bc3423
60ca763
4ffec90
b1db4dc
d1a7353
1b3cec2
d3a6196
026c06d
e1f1a68
36cdb43
9bba449
2448b3e
35d8a49
66bdb34
aaedab9
e3045a1
55767a7
16d5280
4f0bcb5
b787857
86d69dd
bae4cac
ee4492c
c4448ea
cb41f8e
50ce3bd
af9098c
abca08f
17552de
d147067
db87343
e043f22
5540bd1
acac914
0e679f8
09c32e3
147891b
8620f4c
ff23412
d01d844
a2a4831
2f07b12
94a7494
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
andget_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
orDD_PATCH_MODULES=dd_trace_api:false
orpatch_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.
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'll add some code that disables those options for this contrib
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.
ff23412 takes care of disabling via environment variable.
patch_all
is going to be deprecated, so we don't need to disallowdd_trace_api
's use there. #11918