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

fix: logging #1145

Merged
merged 31 commits into from
Jun 20, 2024
Merged

fix: logging #1145

merged 31 commits into from
Jun 20, 2024

Conversation

wass3rw3rk
Copy link
Member

PR is focused on various logging fixes and accomplishes the following:

  1. Injects a logger into the context via existing logging middleware in router/middleware/logger.go
  2. Adds more fields to the request logger (in the logging middleware), such as IDs for resources
  3. Other middleware builds up fields on the injected logger, for example: “build” middleware adds build number and id to the logger and updates the logger in the context - this is to avoid repetition in the handlers where we would have to manually add each.
  4. Request logger is indicator for final state of resource. for example, a 200 on a DELETE for a resource indicates successful deletion, it won’t be logged outside the request logger, but should have all the augmented metadata for a richer log entry. one exception is on creation for resources, since middleware can’t populate something that doesn’t exist, we populate the logger with appropriate fields once the resource has been created and emit a extra log, so you will see a request logger entry (201 status code) and a separate entry that a resource was created with associated ID and such.
  5. Logs creation and modification of resources, including IP, path, and other metadata (see previous points)
  6. Logs read access to sensitive resources, such as secrets
  7. More consistency: always lower case, debug level when entering handler, info/error after a resource was created/changed (unsuccessfully), etc
  8. Additional debug logging in main webhook handler
  9. Adds hook middleware

Note: this PR also addresses some swagger annotations issues discovered (missed in #1139)

api/pipeline/validate.go Show resolved Hide resolved
api/webhook/post.go Show resolved Hide resolved
api/webhook/post.go Show resolved Hide resolved
api/admin/repo.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 16.75127% with 820 lines in your changes missing coverage. Please review.

Project coverage is 52.15%. Comparing base (62c4225) to head (e1c4e37).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
+ Coverage   51.97%   52.15%   +0.17%     
==========================================
  Files         548      550       +2     
  Lines       18794    18833      +39     
==========================================
+ Hits         9768     9822      +54     
+ Misses       8468     8447      -21     
- Partials      558      564       +6     
Files Coverage Δ
api/types/dashboard.go 100.00% <ø> (ø)
compiler/native/native.go 88.63% <100.00%> (ø)
database/build/clean.go 100.00% <100.00%> (ø)
database/build/count.go 100.00% <100.00%> (ø)
database/build/count_deployment.go 100.00% <100.00%> (ø)
database/build/count_org.go 100.00% <100.00%> (ø)
database/build/count_repo.go 100.00% <100.00%> (ø)
database/build/count_status.go 100.00% <100.00%> (ø)
database/build/create.go 73.33% <100.00%> (ø)
database/build/delete.go 100.00% <100.00%> (ø)
... and 290 more

... and 20 files with indirect coverage changes

api/build/enqueue.go Show resolved Hide resolved
api/build/enqueue.go Show resolved Hide resolved
@wass3rw3rk wass3rw3rk marked this pull request as ready for review June 17, 2024 20:20
@wass3rw3rk wass3rw3rk requested a review from a team as a code owner June 17, 2024 20:20
api/admin/build.go Outdated Show resolved Hide resolved
api/build/clean.go Outdated Show resolved Hide resolved
api/build/enqueue.go Outdated Show resolved Hide resolved
api/build/enqueue.go Outdated Show resolved Hide resolved
api/secret/create.go Outdated Show resolved Hide resolved
api/webhook/post.go Outdated Show resolved Hide resolved
api/webhook/post.go Outdated Show resolved Hide resolved
api/webhook/post.go Outdated Show resolved Hide resolved
wass3rw3rk and others added 7 commits June 18, 2024 13:56
removes "in the/from (the) database" wording from log messages within
the database package, they should all have "database": "<database
engine>" as a log field anwyay. also removes it from api package.
ecrupper
ecrupper previously approved these changes Jun 20, 2024
Copy link
Contributor

@plyr4 plyr4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, thanks for the followups

@wass3rw3rk wass3rw3rk merged commit 4fd820a into main Jun 20, 2024
13 of 16 checks passed
@wass3rw3rk wass3rw3rk deleted the chore/logging branch June 20, 2024 19:53
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.

4 participants