-
Notifications
You must be signed in to change notification settings - Fork 2
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
Send GitLab host logs to CloudWatch (#3894) #5187
Send GitLab host logs to CloudWatch (#3894) #5187
Conversation
b1dcea5
to
c9f9d78
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5187 +/- ##
========================================
Coverage 84.41% 84.41%
========================================
Files 149 149
Lines 18305 18305
========================================
Hits 15452 15452
Misses 2853 2853 |
} for path in ['gitaly/gitaly_ruby_json.log', | ||
'gitlab-shell/gitlab-shell.log', | ||
'nginx/gitlab_access.log', | ||
'nginx/gitlab_error.log', | ||
'nginx/gitlab_registry_access.log', | ||
'puma/puma_stderr.log', | ||
'puma/puma_stdout.log'] |
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.
} for path in ['gitaly/gitaly_ruby_json.log', | |
'gitlab-shell/gitlab-shell.log', | |
'nginx/gitlab_access.log', | |
'nginx/gitlab_error.log', | |
'nginx/gitlab_registry_access.log', | |
'puma/puma_stderr.log', | |
'puma/puma_stdout.log'] | |
} for path in [ | |
'gitaly/gitaly_ruby_json.log', | |
'gitlab-shell/gitlab-shell.log', | |
'nginx/gitlab_access.log', | |
'nginx/gitlab_error.log', | |
'nginx/gitlab_registry_access.log', | |
'puma/puma_stderr.log', | |
'puma/puma_stdout.log' | |
] |
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.
Also consider removing the .log
extension from these entries and moving it to the f-strings above, e.g. "file_path": f'/mnt/gitlab/logs/{path}.log',
"file_path": f'/mnt/gitlab/logs/gitlab-rails/{path}', | ||
"log_group_name": "/aws/cwagent/azul-gitlab", | ||
"log_stream_name": f'/mnt/gitlab/logs/gitlab-rails/{path}' | ||
} for path in ['api_json.log', |
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.
Same comments as above
'cron', | ||
'maillog', | ||
'messages', | ||
'secure'] |
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.
Same wrapping issue as above
a1e706a
to
5cb1176
Compare
e98ca13
to
40f1b05
Compare
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.
I can't find precedent for your solution in the codebase or the contributing guide, but I also can't come up with anything better since my previous suggestions didn't work. We should still be consistent though and there's one place where the line breaks are still inconsistent.
"file_path": f'/mnt/gitlab/logs/{path}.log', | ||
"log_group_name": "/aws/cwagent/azul-gitlab", | ||
"log_stream_name": f'/mnt/gitlab/logs/{path}.log' | ||
} for path in |
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.
For consistency with the other sections
} for path in | |
} | |
for path in |
c043736
to
f60fcdc
Compare
0d10bdf
to
56f48e8
Compare
134180c
to
051811d
Compare
'-a', 'fetch-config', | ||
'-m', 'ec2', | ||
'-s', | ||
'-c', 'file:/opt/aws/amazon-cloudwatch-agent/etc/amazon-cloudwatch-agent.json' |
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.
If these options have long versions (--foo
) then we should use them here. Any options that aren't self-explanatory—and short ones (-x
) usually aren't—should be explained in a short comment.
@hannes-ucsc: "The de-bump review count because one of my comments was invalid." |
051811d
to
65d3afb
Compare
29151a4
to
c29c8c3
Compare
c29c8c3
to
6bedce5
Compare
6bedce5
to
89fdaf4
Compare
Connected issues: #3894
Checklist
Author
develop
issues/<GitHub handle of author>/<issue#>-<slug>
partial
label to PR or this PR completely resolves all connected issues1 when the issue title describes a problem, the corresponding PR
title is
Fix:
followed by the issue titleAuthor (reindex, API changes)
r
tag to commit title or this PR does not require reindexingreindex
label to PR or this PR does not require reindexinga
(compatible changes) orA
(incompatible ones) tag to commit title or this PR does not modify the Azul service APIAPI
label to connected issues or this PR does not modify the Azul service APIAuthor (chains)
base
label to the blocking PR or this PR is not chained to another PRchained
label to this PR or this PR is not chained to another PRAuthor (upgrading)
u
tag to commit title or this PR does not require upgradingupgrade
label to PR or this PR does not require upgradingAuthor (operator tasks)
Author (hotfixes)
F
tag to main commit title or this PR does not include permanent fix for a temporary hotfixprod
branch has no temporary hotfixes for any connected issuesAuthor (before every review)
develop
, squashed old fixupsmake requirements_update
or this PR does not touch requirements*.txt, common.mk, Makefile and DockerfileR
tag to commit title or this PR does not touch requirements*.txtreqs
label to PR or this PR does not touch requirements*.txtmake integration_test
passes in personal deployment or this PR does not touch functionality that could break the ITPeer reviewer (after requesting changes)
Uncheck the Author (before every review) checklists.
Peer reviewer (after approval)
Primary reviewer (after requesting changes)
Uncheck the before every review checklists. Update the
N reviews
label.Primary reviewer (after approval)
demo
orno demo
no demo
no sandbox
N reviews
label is accurateOperator (before pushing merge the commit)
reindex
label andr
commit title tagno demo
develop
dev
and addedsandbox
label or PR is labeledno sandbox
anvildev
or PR is labeledno sandbox
sandbox
deployment or PR is labeledno sandbox
anvilbox
deployment or PR is labeledno sandbox
sandbox
deployment or PR is labeledno sandbox
anvilbox
deployment or PR is labeledno sandbox
sandbox
or this PR does not remove catalogs or otherwise causes unreferenced indicesanvilbox
or this PR does not remove catalogs or otherwise causes unreferenced indicessandbox
or this PR does not require reindexingsandbox
anvilbox
or this PR does not require reindexingsandbox
sandbox
or this PR does not require reindexingsandbox
anvilbox
or this PR does not require reindexingsandbox
Operator (after pushing the merge commit)
base
dev.gitlab
componentdev
or PR is labeledno sandbox
anvildev.gitlab
componentanvildev
or PR is labeledno sandbox
dev
1dev
1anvildev
1anvildev
1dev
anvildev
1 When pushing the merge commit is skipped due to the PR being
labelled
no sandbox
, the next build triggered by a PR whose merge commit ispushed determines this checklist item.
Operator (reindex)
dev
or this PR does not remove catalogs or otherwise causes unreferenced indicesanvildev
or this PR does not remove catalogs or otherwise causes unreferenced indicesdev
or this PR does not require reindexinganvildev
or this PR does not require reindexingdev
or this PR does not require reindexinganvildev
or this PR does not require reindexingdev
deployment or this PR does not require reindexinganvildev
deployment or this PR does not require reindexingOperator
prod.gitlab
component'Shorthand for review comments
L
line is too longW
line wrapping is wrongQ
bad quotesF
other formatting problem