Do not use init() in ec2 and ecs packages about aws-xray-sdk-go

[dlsniper]

First of all, thank you both for the quick reply.

From the responses I see that the description of the issue was not as clear as it should have been so I'll address these points below.

These plugins are only for customers who are using ec2, ecs or elastic beanstalk environment for their application.

Agreed.

If you are working on your local machine, you don't need to import these plugins.

This means that while I work on my machine, or run this in a system that's not part of AWS, I need to remember to compile the binaries without including these "plugins".
Now, I can do this in two ways:

• commenting the import statements build the binary and then uncomment them and build again when I deploy to either of those targets
• place them in a standalone file, with a special build tag and then build two distinct binaries, one of these extras or not.

From the language specifications: https://golang.org/ref/spec#Package_initialization :

If a package has imports, the imported packages are initialized before initializing the package itself. If multiple packages import a package, the imported package will be initialized only once.

This effectively means that before I can initialize anything in the main package, any other package that I'll import will be initialized. As such, importing those packages, "plugins", will effectively prevent me from customizing the http client before usage unless I go to some arcane jumps and make sure I'll always initialize my packages and then the AWS SDK before the XRay part initializes.

Furthermore, this is very brittle:

// Every plugin should be imported after "github.com/aws/aws-xray-sdk-go/xray" library.

Due to the nature of the imports, if some other library will import these ahead of the place where I do, this will not work, even if I admit, chances are very small this will happen in real-life cases.

Agreed with @luluzhao that we should keep the convenience afforded by the implicit initialization of these plugins.

I understand why it's appealing to have that convenience, but the trade-off is that users will not have control over how their application behaves.

In my case, I have environment variables which help me detect in which environment the binary is running, with some defaults to fallback to, and for me calling that extra line of code is a lot more convenient than to try and fight off the library with the workaround proposed above.

This change would also be backwards incompatible - making me less inclined to merge in.

I agree this change is backwards incompatible, but for now this SDK is not 1.0 stable yet, so there is still time to change and break things (even if it's not ideal as a user). That's why it's great that you are using semver to tag releases, people using package managers like golang/dep know what to expect, how to link to a known version for them until they are ready to make these changes.

What if, however, we added an additional set of plugins, *_explicit or similar naming, for which the explicit Init functions as modified here were enabled.

Maybe as a transition period change it would be ok, but I think it would make maintenance and usage more complex due to having more packages essentially behaving the same. So I would argue that this should be done for the next months, until 1.0 is near, and before that, remove the old packages.

@dlsniper would you be open to modifying your PR to bring in a new set of plugins instead of modifying the behavior of the existing ones?

Sure, I can do this change as well, but as I mentioned, only if this would not become a permanent one but rather a temporary transition measure. Please let me know if you agree to this and I'll send the patches asap.

Thank you.

[tonyghita]

My team had to do a similar workaround as @dlsniper described. The implicit behavior is very surprising to new users of this library, and I would say un-idiomatic in Go.

It would be better if the use of those packages was made explicit, rather than relying on implicit init behavior.

[luluzhao]

@dlsniper , thank you so much for reaching out to us regarding your issues. These plugins are only for customers who are using ec2, ecs or elastic beanstalk environment for their application. If you are working on your local machine, you don't need to import these plugins. The reason we use import mechanism is that it is very easy and convenient to use. Customers only need to import packages rather than call certain method to implement.

[jamesdbowman]

Agreed with @luluzhao that we should keep the convenience afforded by the implicit initialization of these plugins. This change would also be backwards incompatible - making me less inclined to merge in.

What if, however, we added an additional set of plugins, *_explicit or similar naming, for which the explicit Init functions as modified here were enabled. @dlsniper would you be open to modifying your PR to bring in a new set of plugins instead of modifying the behavior of the existing ones?

[docmerlin]

I agree, implicit loading (like from db/sql) is fine, but implicit reaching out for things that can take a while is a bad idea and means you can't do stuff dynamically.

[jamesdbowman]

Hi @dlsniper,

We are open to making the change in the manner you suggest, adding new plugins as a transition measure and deprecating old ones, and removing the old plugins entirely upon release of 1.0.

We believe that the new plugins should permanently reside under new package names, as the backwards-incompatibility of this change will not be immediately visible to consumers. Directly changing the plugin behavior under the same package name will not result in any compile or runtime errors for consumers, and will silently fail to record AWS metadata on their segments (consumers may not realize they now need to call Init()). Moving the plugins to new package names will reduce the chance of this occurence for consumers upgrading to 1.0+ versions of the SDK.

One option for the new package name: "github.com/aws/aws-xray-sdk-go/awsplugins/ec2", but we're open to other suggestions.

Thank you.

[haotianw465]

I believe we have a path forward here, which is to have the plugins use explicit Init() but under a new namespace to avoid silent breaking for existing customers. I'm closing this for now. We will leave the PR #40 open and you are always welcome to make it up-to-date and we are happy to review and merge it in. But feel free to leave any comment here if you still have any concerns.

Regardless of the PR we will make sure this issue get fixed upon the GA release of the SDK. We cannot provide a timeline here but we will do our best to get the improvement out. Please stay tuned and thank you for your patience.

[noliva]

Is this still going? or is there a fix I haven't found?

[yogiraj07]

Hi @noliva ,
The feature is not merged into master. We plan to to add this in GA release of the SDK. We cannot provide any timeline at this point, however please stay tuned for the updates.