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.

Sign up for GitHub

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

Jump to bottom

chore: Improve and simplify logging #4194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: Improve and simplify logging #4194

wants to merge 1 commit into from

Conversation

m-horky
Copy link
Contributor

@m-horky m-horky commented Aug 19, 2024 •
edited
Loading

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

  • Card ID: CCT-705

This patch will help us with identifying what is happening at given times.

  • Include phase, version and arguments on each phase load. Some of the information may not be necessary, as newer builds of insights-client contain logs that carry the information about phase and version. However, older builds do not, and these bits are valuable when debugging.
  • Disable logging of HTTP responses for collection rules and Inventory host. They are verbose and do not need to be logged in a file every time. The important parts of it are logged elsewhere.

@xiangce xiangce added the client These issues represent work to be done by the "client" team. label Aug 26, 2024
@m-horky m-horky force-pushed the better-logging branch 4 times, most recently from f5e1eee to e44e4be Compare August 29, 2024 11:00
ptoscano
ptoscano reviewed Aug 30, 2024
View reviewed changes
insights/client/connection.py Show resolved Hide resolved
insights/client/connection.py Show resolved Hide resolved
insights/client/connection.py Outdated Show resolved Hide resolved
insights/client/connection.py Show resolved Hide resolved
@xiangce
Copy link
Contributor

xiangce commented Sep 5, 2024

Hi @m-horky - Sorry for bringing this trouble to you, as we just resolved an urgent bug within the repo. Please rebase the master branch of your fork of insights-core and then rebase this branch again. Please reach out to me for any problems when doing the rebase. Apologize again.

ptoscano
ptoscano reviewed Sep 19, 2024
View reviewed changes
insights/client/__init__.py
phase=os.getenv("INSIGHTS_PHASE", "unknown"),
version=package_info.get("VERSION", "unknown"),
path=client.__file__,
arguments=" ".join(sys.argv[1:]),
Copy link
Contributor

Choose a reason for hiding this comment

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

hm are you sure logging sys.argv is a good idea? it might contain credentials, eg --username and --password currently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are not settable via CLI, only through the configuration file. Should we try to filter them anyways, in case someone enters it there by mistake?

insights/client/connection.py Show resolved Hide resolved
ptoscano
ptoscano approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

there's one small nit to potentially change, otherwise this LGTM

@xiangce please review as well, thanks!

insights/client/connection.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 22.22222% with 14 lines in your changes missing coverage. Please review.

Project coverage is 77.16%. Comparing base (0c0355f) to head (aefa1e2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
insights/client/connection.py 17.64% 9 Missing and 5 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4194      +/-   ##
==========================================
- Coverage   77.18%   77.16%   -0.02%     
==========================================
  Files         762      763       +1     
  Lines       41513    41540      +27     
  Branches     8773     8782       +9     
==========================================
+ Hits        32042    32056      +14     
- Misses       8413     8422       +9     
- Partials     1058     1062       +4
Flag Coverage Δ
unittests 77.15% <22.22%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@m-horky
Copy link
Contributor Author

m-horky commented Oct 1, 2024

Also please wait for our QE ACK before merging. Thanks.

xiangce
xiangce approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@xiangce xiangce left a comment

Choose a reason for hiding this comment

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

These logging change looks good to me. And as metioned, let's merge it after geting QE's ack

@m-horky
chore: Improve and simplify logging
aefa1e2 
* Card ID: CCT-705

This patch will help us with identifying what is happening at given
times.

- Include phase, version and arguments on each phase load.
  Some of the information may not be necessary, as newer builds of
  insights-client contain logs that carry the information about phase
  and version. However, older builds do not, and these bits are valuable
  when debugging.

- Include payloads, headers and file attachments in the log message, if
  present. This should make it easier to debug PATCH calls.
  BASIC authentication credentials are not included in these headers,
  the library adds them later when constructing the request.

- Disable logging of HTTP responses for checkin. It is verbose and do
  not need to be logged in a file every time.

Signed-off-by: mhorky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiangce xiangce xiangce approved these changes

@ptoscano ptoscano ptoscano approved these changes

Assignees
No one assigned
Labels
client These issues represent work to be done by the "client" team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m-horky @xiangce @codecov-commenter @ptoscano