Comments (9)
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.
from aws-xray-sdk-go.
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.
from aws-xray-sdk-go.
@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.
from aws-xray-sdk-go.
Agreed with @luluzhao that we should keep the convenience afforded by the implicit init
ialization 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?
from aws-xray-sdk-go.
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.
from aws-xray-sdk-go.
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.
from aws-xray-sdk-go.
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.
from aws-xray-sdk-go.
Is this still going? or is there a fix I haven't found?
from aws-xray-sdk-go.
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.
from aws-xray-sdk-go.
Related Issues (20)
- Mutex deadlock HOT 1
- The v2 version xray doesn't work for dynamodb putItem api HOT 3
- Vulnerabilities in aws-sdk-go v1.18.0 HOT 3
- X-Ray for a lambda function HOT 1
- panic when using timestream QueryWithContext with X-Ray
- Fix vulnerability of google.golang.org/[email protected] module HOT 1
- Use Go SDK V2 HOT 2
- SDK V2 intrumentation of DynamoDB doesn't capture table name HOT 2
- omit URL's password when stringifying URL for segment name HOT 2
- Upgrade from `github.com/golang/protobuf` to `google.golang.org/protobuf` HOT 3
- xray.SqlContext duplicates all non-trivial SQL in trace HOT 2
- Remove the use of github.com/pkg/errors as its moved to Archived. HOT 1
- How do I add a custom detector to allow seamless snowflake integration? HOT 2
- Segment silently fails to flush when a subsegment is created with a closed parent HOT 1
- Fix vulnerability of golang.org/x/[email protected] CVE-2023-45288 CWE-770 HOT 2
- UDP message length too long for segments send to daemon HOT 1
- instance metadata related error when running ecs sidecar container for xray HOT 1
- using x-ray with aws-sdk-v2 makes the binary files bigger than expected HOT 1
- Tests Failing -- `TestBadRoundTripDial` HOT 1
- Migrate xray sampling from aws sdk for go v1 to v2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from aws-xray-sdk-go.