Ability to add tracing options to auto-wired libraries

[mlund01]

In search of being able to set the service name on the tracer of my database/sql client, it came to my attention that there is no current way to set the options through orchestrion. After digging a little deeper into the internals, I noticed the underlying tracer function (coming from gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql) is variadic, with the ability to pass 0 or many options, so it is pretty straightforward to add, if only there is a way to tell orchestrion what to add as options.

Proposal

I would like to propose adding another "magic comment" in orchestrion called //dd:options that can be used directly above an instantiation of one of the supported auto-wire libs. It would be used like so,

package main

import "database/sql"

func main() {
	//dd:options service:test-name tag:my_key:my_val
	db, err := sql.Open("driver", "connectionString")
}

Doing so would generate the following code,

package main

import "github.com/datadog/orchestrion/instrument"

func register() {
	//dd:options service:test-name tag:my_key:my_val
	//dd:startwrap
	db, err := instrument.Open("driver", "connString", instrument.SqlWithServiceName("test-name"), instrument.SqlWithCustomTag("my_key", "my_value"))
	//dd:endwrap
}

This proposal requires the instrument.Open function that currently accepts 2 params and is non-variadic to accept 2+ params with the last param being variadic and accept 0 or many options. This function is for database/sql, but the same general need applies to the other supported libs.

Some other questions that should be discussed and answered,

• one vs many entries on the comment line: Some options support passing many (such as custom tags... I think anyways), while others should only be passed once (like service name). Should orchestrion be smart enough to know which ones support one vs. many and throw an error if invalid?
• invalid options: if an invalid/unsupported option is passed in the magic comment, should orchestrion throw an error, inform the user, or do nothing?

---

I got a bit ahead of myself and put together a PR, but can change as necessary if the broader proposal is accepted.

The PR: #60

[mlund01]

@ahmed-mez thoughts on this?

[RomainMuller]

That sounds like a reasonable feature to have. There's in my opinion some stuff regarding the API there that could use a little tweaking... Like to a certain extent you could envision the service: configuration as "a special-named tag", and in that case, it'd seem like all you're looking for here is to specify additional/override tags... So maybe just //dd:tags key:value key:value ... would do?

The implementation could have special behavior when key is service (and the other reserved tag names).

[RomainMuller]

I also just realized that if you add //dd:span on a function that is otherwise instrumented (e.g, net/http handlers), the tags provided to that directive are appended to the list of tags from the standard instrumentation there... Feels like either that idiom is wrong and should be replaced with a new //dd:tags, or that the //dd:span directive should be useable to serve this scenario as well.

[mlund01]

That sounds like a reasonable feature to have. There's in my opinion some stuff regarding the API there that could use a little tweaking... Like to a certain extent you could envision the service: configuration as "a special-named tag", and in that case, it'd seem like all you're looking for here is to specify additional/override tags... So maybe just //dd:tags key:value key:value ... would do?

The implementation could have special behavior when key is service (and the other reserved tag names).

I like this suggestion, especially the fact that by having "special" tags, it implies "all-other" tags are just normal tags, and there is no need to prepend tag to the latter case.

[mlund01]

I also just realized that if you add //dd:span on a function that is otherwise instrumented (e.g, net/http handlers), the tags provided to that directive are appended to the list of tags from the standard instrumentation there... Feels like either that idiom is wrong and should be replaced with a new //dd:tags, or that the //dd:span directive should be useable to serve this scenario as well.

For the sake of simplicity, I think //dd:span could be used to serve this scenario as well, without any need to add a //dd:tags field. Now that I think of it, adding a //dd:tags option is a bit redundant with the //dd:span option. The former focuses on the tags that will be tied to the span, while the latter subjects the span itself as the main object, with the tags as just properties that go with it. I think the //dd:span is more at the heart of what this toolset provides. Add the "special" tags feature and it all rolls up into one simple, clean approach.