-
Notifications
You must be signed in to change notification settings - Fork 316
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
Instrument dd-trace-api #5145
Merged
+421
−0
Merged
Instrument dd-trace-api #5145
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
2ec3ab4
initial poc
bengl 5ea78d8
get the inject test working
bengl d88b02c
support object mapping for callback args
bengl 0156d2f
start adding telemetry
bengl 6a27dd7
parity on sending and receiving sides
bengl e99fbdf
initial dd-trace-api test
bengl 1a846b3
rest of dd-trace-api testing
bengl 5f3779b
quick test refactor
bengl 50b4952
support span context methods
bengl 41102aa
lint
bengl 13e11b1
hoist counter tag
bengl 09d3fa1
add plugin test
bengl 85fe6d9
replace -> replaceAll
bengl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
'use strict' | ||
|
||
const Plugin = require('../../dd-trace/src/plugins/plugin') | ||
const telemetryMetrics = require('../../dd-trace/src/telemetry/metrics') | ||
const apiMetrics = telemetryMetrics.manager.namespace('tracers') | ||
|
||
// api ==> here | ||
const objectMap = new WeakMap() | ||
|
||
const injectionEnabledTag = | ||
`injection_enabled:${process.env.DD_INJECTION_ENABLED ? 'yes' : 'no'}` | ||
|
||
module.exports = class DdTraceApiPlugin extends Plugin { | ||
static get id () { | ||
return 'dd-trace-api' | ||
} | ||
|
||
constructor (...args) { | ||
super(...args) | ||
|
||
const tracer = this._tracer | ||
|
||
this.addSub('datadog-api:v1:tracerinit', ({ proxy }) => { | ||
const proxyVal = proxy() | ||
objectMap.set(proxyVal, tracer) | ||
objectMap.set(proxyVal.appsec, tracer.appsec) | ||
objectMap.set(proxyVal.dogstatsd, tracer.dogstatsd) | ||
}) | ||
|
||
const handleEvent = (name) => { | ||
const counter = apiMetrics.count('dd_trace_api.called', [ | ||
`name:${name.replaceAll(':', '.')}`, | ||
'api_version:v1', | ||
injectionEnabledTag | ||
]) | ||
|
||
// For v1, APIs are 1:1 with their internal equivalents, so we can just | ||
// call the internal method directly. That's what we do here unless we | ||
// want to override. As the API evolves, this may change. | ||
this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, proxy, revProxy }) => { | ||
counter.inc() | ||
|
||
if (name.includes(':')) { | ||
name = name.split(':').pop() | ||
} | ||
|
||
if (objectMap.has(self)) { | ||
self = objectMap.get(self) | ||
} | ||
|
||
for (let i = 0; i < args.length; i++) { | ||
if (objectMap.has(args[i])) { | ||
args[i] = objectMap.get(args[i]) | ||
} | ||
if (typeof args[i] === 'function') { | ||
const orig = args[i] | ||
args[i] = (...fnArgs) => { | ||
for (let j = 0; j < fnArgs.length; j++) { | ||
if (revProxy && revProxy[j]) { | ||
const proxyVal = revProxy[j]() | ||
objectMap.set(proxyVal, fnArgs[j]) | ||
fnArgs[j] = proxyVal | ||
} | ||
} | ||
// TODO do we need to apply(this, ...) here? | ||
return orig(...fnArgs) | ||
} | ||
} | ||
} | ||
|
||
try { | ||
ret.value = self[name](...args) | ||
if (proxy) { | ||
const proxyVal = proxy() | ||
objectMap.set(proxyVal, ret.value) | ||
ret.value = proxyVal | ||
} else if (ret.value && typeof ret.value === 'object') { | ||
throw new TypeError(`Objects need proxies when returned via API (${name})`) | ||
} | ||
} catch (e) { | ||
ret.error = e | ||
} | ||
}) | ||
} | ||
|
||
// handleEvent('configure') | ||
handleEvent('startSpan') | ||
handleEvent('wrap') | ||
handleEvent('trace') | ||
handleEvent('inject') | ||
handleEvent('extract') | ||
handleEvent('getRumData') | ||
handleEvent('profilerStarted') | ||
handleEvent('context:toTraceId') | ||
handleEvent('context:toSpanId') | ||
handleEvent('context:toTraceparent') | ||
handleEvent('span:context') | ||
handleEvent('span:setTag') | ||
handleEvent('span:addTags') | ||
handleEvent('span:finish') | ||
handleEvent('span:addLink') | ||
handleEvent('scope') | ||
handleEvent('scope:activate') | ||
handleEvent('scope:active') | ||
handleEvent('scope:bind') | ||
handleEvent('appsec:blockRequest') | ||
handleEvent('appsec:isUserBlocked') | ||
handleEvent('appsec:setUser') | ||
handleEvent('appsec:trackCustomEvent') | ||
handleEvent('appsec:trackUserLoginFailureEvent') | ||
handleEvent('appsec:trackUserLoginSuccessEvent') | ||
handleEvent('dogstatsd:decrement') | ||
handleEvent('dogstatsd:distribution') | ||
handleEvent('dogstatsd:flush') | ||
handleEvent('dogstatsd:gauge') | ||
handleEvent('dogstatsd:histogram') | ||
handleEvent('dogstatsd:increment') | ||
handleEvent('use') | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
what is this block trying to do here?
i have been reviewing this in the context of the
dd-trace-api-js
implementation, and everything in these handlers around this block makes sense to me (in terms of setting and grabbing the proxy obj value fromself
, and calling the mapped function on the tracer), but I'm unsure what this block is doing.in what case would we check the arguments to the function against the
objectMap
? what isrevProxy
(i see on the publisher side & tests it's just defaulted to an empty list, which doesn't help here 😅)? why do we need to wrap afunction
argument (and, maybe it's arguments)?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.
It's preserving the object map across all function calls that happen through our message bus (i.e. diagnostics channel). That means objects on the API side will never be real objects returned from real API calls.
We want to map these to "real" objects from the internal/dd-trace API so that a "real" value is never exposed to the user via dd-trace-api. I sometimes call this concept "the firewall". We need the firewall because we never want to expose an internal API to the user by accident, or else the whole premise of keeping the API separate just falls apart.
The object map has "dummy" objects as keys, and "real" objects as values. To maintain the firewall, we map them later on (see what we do later on in code with
ret.value
). When an API call is made theproxy
property contains a function that returns a "dummy" object to map to. This is how we populate the object map.Now, suppose one of these dummy objects, having already been mapped, is passed in to one of our APIs (take
tracer.inject(span, carrier)
as an example, where span was returned from one of our APIs so it's a mapped object). In order to make sure that the underlying "real API" only receives "real" objects, we need to map all of these through the object map. This would give us some nice code that looks something like this:That would preserve our firewall with respect to arguments passed to these API calls, and indeed, constitutes the first few lines you reference here.
Some of our API calls can take in callbacks. In this case, we need to do everything in reverse; instead of mapping return values, we need to map function arguments. A simple way to think of why this is necessary is to consider that callback arguments are effectively return values (google "continuation passing style"). An easy example API here is
tracer.trace('name', options, () => { /* do stuff to trace */ })
.So, to deal with this in our loop, we first check if any given argument is a function. That means it's a callback so we have to jump into this reverse-mapping procedure. We replace the function with a wrapper function that passes any relevant args through relevant
revProxy
array functions, which need to give us dummy objects to pass through, much likeproxy
does for regular return values. We then map then in the object map, again, just like we do for return values. Then we're good to call the original/wrapped function.In summary (and as a tl;dr, if you want), we have to map return values, and we have to unmap them when they're passed in as arguments. For callbacks, we have to do the reverse of those two operations. And that's about it.
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.
This actually makes a lot of sense!! I think the overarching idea i wasn't connecting here was the whole "firewall" approach, which i get now in obfuscating the actual return values of things like span, scope, etc. from the tracer core api. i actually find that
tracer.inject
example a simple and understandable one for this, WRT passing these objects back to other tracer APIs!it further helped seeing how it's actually implemented using
defaultFun
to signal this as theproxy
value, and then through stuff likegetSpan
returns a dummy span with only the methods/properties we want to expose, and so on indd-trace-api-js
.i think maybe a small comment here could help future readers like myself - maybe just linking to the explanation above?