dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Add decorator syntax for tracing OOP code

Open matt-casey opened this issue 5 years ago • 4 comments
trafficstars

What does this PR do?

This PR adds a new decorator syntax to dd-trace-js.

import { trace } from 'dd-trace-js';

@trace({/* Optional Configuration */})
class TraceAllOfMyMethods {
    public thisWillBeTraced() {}

    private async thisWillAlsoBeTraced() {}
}

class TraceSpecificMethods {
    @trace({/* Optional Configuration */})
    public thisWillBeTraced() {}

    private async thisWillNotBeTraced() {}
}

Motivation

One of the primary motivations was to lessen the amount of boilerplate needed to trace our code. As we use a lot of OOP conventions, decorator syntax made a lot of sense for us. It allows tracing code to be added and removed without touching any of the business logic code.

I've written in more detail about the extra tooling we found worthwhile, and we open sourced some of our code as an npm package.

When I shared the blog post and library in the public datadog slack, @bengl requested upstreaming the decorator portion of the code: Screen Shot 2020-02-03 at 2 59 20 PM

If this could be upstreamed, it would remove most of the need for maintaining our own library on top of dd-trace-js, so it seemed a worthwhile use of my hack day this past Friday.

Plugin Checklist

This isn't a plugin, however the checklist still seems relevant.

Additional Notes

This required adding extra babel tooling in order to make use of the decorator syntax.

I exported this at the top level of dd-trace-js, but it might make sense to live somewhere else. It didn't quite fit the same pattern as the plugins, however.

There are a few other helper functions in the gamechanger/datadog-apm library, but they felt outside of the scope of this PR. It might make sense to add them later if others find them useful.

matt-casey avatar Mar 02 '20 20:03 matt-casey

Thanks for the PR, I'll take a look!

Regarding the above about upstreaming, I'd definitely recommend it whenever possible to avoid forks with different capabilities. I think all the features of the fork make sense, except maybe the "root span" helper as right now there are some cases that make this difficult to do in a predictable way, but opening the discussion would still have a lot of value.

rochdev avatar Mar 02 '20 21:03 rochdev

Do the TypeScript decorators work according to the official proposal? If that's the case, I think Babel would not be needed for the test.

rochdev avatar Mar 02 '20 21:03 rochdev

Typescript decorators are based on the stage 2 proposal that you linked to.

The typescript test does not require babel, and isn't using it, but does require experimentalDecorators: true in tsconfig.json in order to work.

matt-casey avatar Mar 02 '20 21:03 matt-casey

Last few weeks have been busy for me, but had some time today to review your comments @rochdev.

I'll work on reusing tracer.wrap for the decorator as you suggested. I responded to your other two questions above.

matt-casey avatar Apr 24 '20 20:04 matt-casey

Hey @matt-casey, we're looking at closing old PRs out. This on has a few merge conflicts and looks like it has some remaining TODOs. Is it something you'd be interested in patching up or is it safe to close?

tlhunter avatar Feb 21 '23 23:02 tlhunter

I'll close this for now but feel free to reopen if you'd like to drive it on home.

tlhunter avatar Dec 20 '23 00:12 tlhunter