-
Notifications
You must be signed in to change notification settings - Fork 118
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
Do not use init() in ec2 and ecs packages #38
Comments
This allows a user to import plugins on local development environments which do not have access to AWS metadata without stalling the process at boot time. Each of the three plugins will need to be imported and initialized manually. The API will safeguard against multiple requests but it's not goroutine safe.
@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. |
Agreed with @luluzhao that we should keep the convenience afforded by the implicit What if, however, we added an additional set of plugins, |
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.
Agreed.
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".
From the language specifications: https://golang.org/ref/spec#Package_initialization :
This effectively means that before I can initialize anything in the Furthermore, this is very brittle:
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.
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.
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
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.
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. |
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 |
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. |
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 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 One option for the new package name: Thank you. |
I believe we have a path forward here, which is to have the plugins use explicit 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. |
Is this still going? or is there a fix I haven't found? |
Hi @noliva , |
Adding the following two imports to my application:
will make it take up to 10 seconds to timeout before it can continue loading when I'm working on my local machine and not in an EC2 or ECS instance.
I would like to argue for removing the implicit behaviour those two imports have and manually adding an idempotent
Init()
function on each package which will allow users to call those functions whenever needed. It might not make for a "nice" experience but I'll take adding an extra couple of lines of code any day over 10 seconds of waiting time on application boot any single day.The text was updated successfully, but these errors were encountered: