-
Notifications
You must be signed in to change notification settings - Fork 105
Support loading Python agent through sw-python CLI #156
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
Support loading Python agent through sw-python CLI #156
Conversation
Signed-off-by: YihaoChen <[email protected]>
Signed-off-by: YihaoChen <[email protected]>
Looking at the docs just makes me feel exciting about this new feature that I've been planning for so long, and I believe this feature will be the last piece before our 1.0 GA. Look forward to this feature and our 1.0 GA version, soon! |
Let me review and try this new CLI tomorrow 🤩 |
That's what I thought! This feature will ease a lot of work in deployment.
I also need to run it more in a Linux env tmr, I'm struggling with my WSL and Windows doesn't communicate to each other properly over localhost... So my local test coverage is quite limited for now (not able to deploy gunicorn on windows etc.). After I verify most cases, I could use some help testing this CLI on different OS and shells and see if it breaks for some corner cases. |
Signed-off-by: YihaoChen <[email protected]>
Signed-off-by: YihaoChen <[email protected]>
Use GHA to verify running in various env. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Superskyyy , I just test (on MacOS) and review the codes, and left some comments.
Since you did a lot of test cases, it would be perfect if you can add those cases into our test matrix in GitHub Actions, if you find any trouble in adding that, ping me at anytime, I can offer my help!
The currently supported method is to provide the environment variables listed | ||
in [EnvVars Doc](EnvVars.md) as instructed in the [README](../README.md). | ||
|
||
#### Through a sw-config.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we should note when implementing this is that we've been trying to minimize our dependencies as an agent, so we'd better choose a reasonable file format that Python built-in libs can parse, instead of bringing another library, to avoid version conflicts with user applications, yaml
is a common case where users may already use and we should not require dependency upgrade or downgrade.
Fix error not raised in runner. Signed-off-by: YihaoChen <[email protected]>
I'm actually thinking what if I change/ duplicate our existing plugin unit tests to use the Idk if we need to keep the old tests that directly call |
What I think is to replace all existing tests with the |
That could be done in following pull requests though. |
Signed-off-by: YihaoChen <[email protected]>
Co-authored-by: kezhenxu94 <[email protected]>
Adopt suggested doc changes. Co-authored-by: kezhenxu94 <[email protected]>
Fix missing punctuation. Signed-off-by: YihaoChen <[email protected]>
…yy/skywalking-python into non-intrusive-bootstrap
Remove the version number for CLI now. Signed-off-by: YihaoChen <[email protected]>
Signed-off-by: YihaoChen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Superskyyy very much for doing this!! 🎉
This PR adds functionality of non-intrusive integration CLI for the Python agent.
Though the code is completed, as a dangerous feature that modifies Python bootstrap I will need to test it a bit more before it's ready for review.
closes apache/skywalking#7381