-
Notifications
You must be signed in to change notification settings - Fork 921
feat: Adding aws cloudwatch external logging #6331
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
feat: Adding aws cloudwatch external logging #6331
Conversation
|
@aylei I saw you assigned the original issue, hopefully you hadn't spent much time on it, but figured I'd take a crack |
|
Thanks! I have not looked into the issue yet😉 |
aylei
left a comment
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.
Thanks for adding this @clayrosenthal ! The changes look good to me, just left some nits
sky/logs/aws.py
Outdated
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.
| # 'tag': |
Nit: looks like we don't need tag in inputs
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.
Yeah my bad, can remove this
sky/logs/aws.py
Outdated
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.
Nit: shall we keep the defaults in config init for coherence?
sky/logs/aws.py
Outdated
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.
| log_group_name: Optional[str] = None | |
| log_stream_prefix: Optional[str] = 'skypilot-' | |
| log_group_name: str = 'skypilot-logs' | |
| log_stream_prefix: str = 'skypilot-' |
Will this work?
|
/smoke-test -k test_log_collection_to_aws_cloudwatch |
|
The UT failure looks irrelevant, I will take a look. Could you fix the document build @clayrosenthal ? |
ddc69aa to
c19f6d0
Compare
Signed-off-by: Clay Rosenthal <clayros@amazon.com>
c19f6d0 to
5f6bb7a
Compare
|
Ok should be all fixed now, thanks @aylei ! |
|
/smoke-test -k test_log_collection_to_aws_cloudwatch |
Signed-off-by: Aylei <rayingecho@gmail.com>
|
Hi @clayrosenthal ! I notice that there is an issue of fallback logs: And looks like the logs will always get output to fallback logs: Do you have any ideas about this? @clayrosenthal |
|
Huh, I can try and change the fallback log options, but maybe not necessary as the log files should already exist. But do your AWS access have permissions to write to cloudwatch? |
Yes, the log was also sent to cloudwatch in this case. |
|
/smoke-test -k test_log_collection_to_aws_cloudwatch |
Signed-off-by: Aylei <rayingecho@gmail.com>
|
/smoke-test -k test_log_collection_to_aws_cloudwatch |
|
Let's merge this first and keep the fallback logs issue as a follow-up, thanks for the making this happen @clayrosenthal ! |
Adding AWS cloudwatch as an option for external logging
closes #6167
added tests for the config and logging in
tests/sky/logs/test_aws.pyTested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)