Skip to content
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

Explicit plugins #81

Merged
merged 3 commits into from
Jan 31, 2019
Merged

Explicit plugins #81

merged 3 commits into from
Jan 31, 2019

Conversation

muirdm
Copy link
Contributor

@muirdm muirdm commented Jan 25, 2019

Fixes #38, supersedes #40.

Add explicit versions of the aws metadata plugins so users can conditionally load the plugins in prod/dev.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Muir Manders added 2 commits January 25, 2019 15:49
The existing aws plugins install their metadata at package load
time. They aren't meant to be loaded in test/development environments,
but there is no good way to only load them in production since you
can't conditionally load a package.

- move the plugins under a new /awsplugins path and change init() to
  Init() so user must explicitly call a method to load the plugin.
- leave the old plugins in place, delegating to the new plugins. i
  also left the "Origin" constants since those are part of the
  packages' public APIs.
@muirdm
Copy link
Contributor Author

muirdm commented Jan 29, 2019

Hello - wondering if anyone is available to work with me on this? I have a couple more PRs after this to fix the logger and package load side effects, so I don't want things to sit around too long and get out of sync with master. Thanks.

@luluzhao
Copy link
Contributor

@muirrn , thank you so much for sending out PR to us. Feel free to send more PRs in future and we will prioritize them. Since the explicit loading plugins is an breaking change so we want to merge it once we are at GA.

@muirdm
Copy link
Contributor Author

muirdm commented Jan 30, 2019

Thank you for responding.

This is not a breaking change because it maintains the same behavior for the existing plugins. The explicit plugins are located under a new path /awsplugins instead of /plugins (as suggested by #38 (comment)).

@luluzhao
Copy link
Contributor

Hi @muirrn , the reason I said it is a breaking change since if customer update the version of X-Ray Go SDK, the old way(automatically start adding information) will not work. The explicit way need customer to modify their application and call Init() method to start tracking information.

@muirdm
Copy link
Contributor Author

muirdm commented Jan 31, 2019

There is no change in behavior for existing users. The existing plugins are still initialized at package load time automatically.

If the user elects to import the new plugins under /awsplugins then they have to explicitly call Init().

@luluzhao
Copy link
Contributor

@muirrn , I understand your argument which is fair. Could you also update the README about how to use the new one for this PR so that if customer want to start using it, they can manually check-in this PR since we haven't done a release for this feature.

@muirdm
Copy link
Contributor Author

muirdm commented Jan 31, 2019

I added a note in the README.md about the old and new plugins.

@luluzhao luluzhao merged commit 0aa94d4 into aws:master Jan 31, 2019
@luluzhao
Copy link
Contributor

@muirrn I Merged your PR and thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants