Propose Datadog's auto-instrumentation libraries as a starting point #41
Conversation
bhs
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
reyang,
SergeyKanzhelev,
songy23,
tedsuo and
yurishkuro
as code owners
|
I have reviewed dd-trace-java and chatted with @tylerbenson, and I agree that it would be a good basis for OpenTelemetry Java auto instrumentation 👍 |
| * For some (hopefully brief) period: | ||
| * When new plugins are added to Datadog's (original) repo, merge them over into the `auto-instr-foo` repo | ||
| * Allow Datadog end-users to bind to either for some period of time (ultimately Datadog's decision on timeline here, and does not prevent other tracers from using `auto-instr-foo`) | ||
| * Once Datadog has confidence, replace the original `dd-trace-foo` repo with the thin wrapper from above. (Note that Datadog end-users should not need to care about this refactor) |
There was a problem hiding this comment.
This point requires close cooperation with / adds a dependency on Datadog as an organization. Rather than a instruction for instrumentation creators, could it be worded as a non-normative note about what Datadog will do? Of course it would make sense to at least require the creator to notify Datadog of the creation/production-readiness of such an instrumentation.
There was a problem hiding this comment.
@Oberon00 great point – I have tried to address that in the revision I just pushed.
|
I reviewed dd-trace-dotnet and agree that it would be suitable for this, and also a pretty straightforward port. |
|
|
||
| ### The overarching (technical) process, per-language | ||
|
|
||
| * Start with [the Datadog `dd-trace-foo` tracers](https://github.com/DataDog?utf8=✓&q=dd-trace&type=source&language=) |
There was a problem hiding this comment.
How about languages that DD do not support? This proposal seems very DD centric and bind OT to DD implementations of tracer.
There was a problem hiding this comment.
Great point. I've added a note below.
|
Since there don't seem to be any objections (and since all four members of the OpenTelemetry TC has approved this), I am planning to merge this PR within 24h... thanks for the comments thus far. |
|
@bhs thanks for putting this together! |
There was a problem hiding this comment.
Did we complete the review/bakeoff process for Java? IIRC the next step after the prior art for Java was submitted was going to be trying to figure out which prior art to incorporate, so I'm surprised to see this being pushed through so quickly within a 72 hour period.
I see the language saying that Datadog would prefer that it be a uniform strategy, and that other options have been listed under "Prior Art", but I think this requires more than 72 hours of discussion.
| * Note that, by design, this is not expected to affect Datadog end-users | ||
| * Moved repo is GA’d: all new plugins (and improvements to the auto-instrumentation core) happen in the `auto-instr-foo` repo | ||
|
|
||
| There are some languages which will have OpenTelemetry support before there's Datadog `dd-trace-foo` support. In those situations, we will fall back to the requirements in this OTEP and leave the technical determinations up to the language SIG and the OpenTelemetry TC. |
There was a problem hiding this comment.
This is still very vague. It do not describes:
- How the auto-instrumentation should work.
- What should be instrumented automatically.
- What kind of metadata should be included, etc.
I feel like this is written as: "Just incorporate whatever company BigTech is doing, and then worry about specs later. YOLO." Do not understand me wrong, I <3 DataDog and their tools, I have written some integrations between Erlang OpenCensus implementation and their service, but I do not see this as a suitable proposal, at least not in this form and wording.
There was a problem hiding this comment.
The trouble with automatic instrumentation is that the details vary – often significantly – from language to language. The RFC here is written at "a consistent altitude" which I will acknowledge is vague about "the how". IMO, though, it would be a mistake to get into those details in this doc since it would create so much sprawl and distract from the directional goals.
The actual telemetry emitted by automatic instrumentation is not supposed to be distinguishable (in terms of metadata/etc) from "whitebox" instrumentation.
I will add a few sentences about this when I have a chance, probably later this afternoon.
After further discussion with my team, I think that we needn't block merging this, but we do need some clarification about the approval process for changes to the repo -- how will governance work for contribution of additional auto-instrumentation plugins for frameworks? I see there's something about how to copy over changes from datadog into the opentelemetry project, but is the intention for most new plugins to go first into the opentelemetry project? also, this seems to fully subsume #7 as intended wrt O(1) but not 0 code change or O(N) instrumentation, which is good. |
|
FWIW we're interpreting this on the .NET SIG as being a "yes, and" approach to instrumentation plugins - we currently ship with support for auto-instrumentation of several libraries through imports and would like to continue that. The eventual location of those plugins can change, but until there's more clear-cut direction (probably until there's a stable release of OTel) we'd like to keep them alongside the main repository. |
|
@lizthegrey re: "IIRC the next step after the prior art for Java was submitted was going to be trying to figure out which prior art to incorporate" I briefly mention that in the PR description above... this is a deviation from that plan, but IMO a worthwhile one. Of course it's fair game to make improvements to any of this code (it will certainly require some changes just to generalize things). As for your question about the approval/governance process: good point, I'll add something to this later today. TL;DR I think the language SIG structure should carry over to these repos (and I also think it's important that these ported auto-instrumentation repos only use the public APIs of the "core" repos – that's an important safeguard in keeping with OTel's overall goal of loose coupling). |
|
@lizthegrey PTAL at the notes I added re: governance. I am still planning to merge this PR later today given the enthusiasm for the general direction and the approvals from the full OTel TC... there is a lot of work to do to flesh out the many details, but I don't consider that the job of this OTEP/RFC (where my goal is more to establish high-level requirements and a basic strategy and approach we can align around). If there is further wordsmithing to do here (or further clarifications we can add to help with alignment), I obviously welcome further PRs against this OTEP – please just mark me as a reviewer and we can take it from there. |
For context, I have been talking with folks from Datadog about this for a bit now... we have a rough technical plan that we feel is feasible, and they are enthusiastic about supporting the broader OSS community by donating the (IMO excellent) work they've done so far on their
dd-trace-footracers which do a lot of auto-instrumentation in the style described by this OTEP.I also want to note and acknowledge that there was a meeting a few weeks ago to discuss possibilities for auto-instrumentation in the JVM world... notes are here. I would like to cc @trask specifically since he has deep domain knowledge in this area and I'd appreciate his 👀.
I'm also going to explicitly cc @open-telemetry/technical-committee – I'm listing you as the "Reviewers" here since ultimately this is your call (IIUC).
Finally, I would love it if people could take a look and add comments within 72h (or, if that's not possible, comment briefly with a deadline you can stick to).
Thanks!