Update Sphinx docs #97

Darwinkel · 2022-12-16T14:30:53Z

Changes

This PR makes some minor tweaks to the documentation, adds GitHub templates, and adds some logging guidelines to the documentation.

Issue ticket number and link

Linked.

Proof

Checklist for author(s):

This PR comes from a feature or hotfix branch, in line with our git branching strategy;
This PR is "bite-sized" and only focuses on a single issue, problem, or feature;
I am not reinventing the wheel: there is no high-quality library that already has this feature;
I have changed the example .env files if I added, removed, or changed any config options, and I have informed others that they need to modify their .env files if required;
I have performed a self-review of my own code;
I have commented my code, particularly in hard-to-understand areas;
I have made corresponding changes to the documentation, if necessary;
I have written unit, integration, and end-to-end tests for the change that I made;

If a non-trivial PR:

This PR is part of a milestone and has appropriate labels;
This PR is properly linked to the project board (either directly or via an issue);
I have added screenshots or some other proof that my code does what it is supposed to do;

## Checklist for functional reviewer(s):
- [ ] If a non-trivial PR: This PR is properly linked to an issue on the project board;
- [ ] I have checked out this branch, and successfully ran `make kat`;
- [ ] I have ran `make test-rf` and all end-to-end Robot Framework tests pass;
- [ ] I confirmed that the PR's advertised `feature` or `hotfix` works as intended;
- [ ] I confirmed that there are no unintended functional regressions in this branch;

### What works:
* _bullet point + screenshot (if useful) per tested functionality_

### What doesn't work:
* _bullet point + screenshot (if useful) per tested functionality_

### Bug or feature?:
* _bullet point + screenshot (if useful) if it is unclear whether something is a bug or an intended feature._

## Checklist for code reviewer(s):
- [ ] The code passes the CI tests and linters;
- [ ] The code does not bypass authentication or security mechanisms;
- [ ] The code does not introduce any dependency on a library that has not been properly vetted;
- [ ] The code does not violate Model-View-Template and our other architectural principles;
- [ ] The code contains docstrings, comments, and documentation where needed;
- [ ] The code prioritizes readability over performance where appropriate;
- [ ] The code conforms to our agreed coding standards.

dekkers · 2022-12-22T14:13:29Z

docs/source/guidelines/logging.rst

+Some general guidelines for how to log within your applications:
+
+- Log after, not before. This communicates what operation was performed, and what the result was.
+


I don't agree that logging should not be done before. Not logging information before an action means you miss crucial information when things go wrong and an exception is thrown because you haven't logged anything. Also if something takes a long time / hangs it's harder to figure out what it is.

I had a lot of trouble with debugging keiko when subprocess.run caused an exception. We had no idea what exactly keiko was executing, because that was only logged after and was thus never logged. So the time we actually needed the log information it wasn't there.

I'm ok with supplementing/updating this

dekkers · 2022-12-22T14:16:01Z

docs/source/guidelines/logging.rst

+- Separate parameters and messages.
+  This will make sure that logs are parseable, searchable, easy to extend, and read.
+  A python package called `structlog <https://www.structlog.org/>`_ can help with this.
+  This is one example on how to separate parameters and messages, you can also decide to for instance use a json output:


Either we make the decision to use structlog, make the changes to use it everywhere and ask all our contributores to use it, or we don't. Telling that there is a tool that might be useful is not something that is useful to put in contribution guidelines.

Since the piece wasn't written with KAT in mind, we should remove this and decide what kind of output we want.

dekkers · 2022-12-22T14:21:12Z

docs/source/guidelines/logging.rst

+  information. So no email addresses, names, credit card numbers, etc.
+
+- Always log in UTC, log in milliseconds. Synchronize your servers with a NTP
+  daemon.


This is also not really useful for our contributor guidelines.

I'm ok with removing this

dekkers · 2022-12-22T14:22:13Z

docs/source/guidelines/logging.rst

+
+..
+    - TODO: request guid, see if this is already done by log aggregation of them
+      used cloud provider.


This also doesn't really make much sense in the context of KAT.

Yes remove this

dekkers · 2022-12-22T14:28:52Z

docs/source/guidelines/logging.rst

+  went wrong. These severities need attention and fixing.
+
+* ``INFO`` is for business, ``DEBUG`` is for technology. The ``INFO`` log should look
+  like a book.


I don't agree with this. They are levels, not a categorisation. DEBUG is less important information than INFO, there might still be "technology" stuff that is important enough to log at INFO level.

I think the Python definition of the levels make a lot more sense: https://docs.python.org/3/howto/logging.html

And I think it's also important as an Open Source project that has lots of different users to not just redefine what these log levels mean.

I'm, ok with redefining this using the python log levels

dekkers · 2022-12-22T14:43:43Z

docs/source/guidelines/logging.rst

+
+    # Do this
+    some_stuff()
+    log.info("Did stuff with %s [url=%s]", url, url)


This should be "Did stuff [url=%s]", logging the url twice isn't really useful.

Another thing that might be useful to add is to use the formatting of logger such as is done here and not f-strings, especially for debug logging. The reason is that if the log level is disabled logger won't construct the string, but a f-string as argument will always need to be constructed.

dekkers · 2022-12-22T14:58:16Z

.github/ISSUE_TEMPLATE/bug_report.md

+If applicable, add screenshots to help explain your problem.
+
+**OpenKAT version**
+Should contain at least the commit or release of Rocky. If you think another module may be the cause of the bug, note that module's commit or release here as well.


I think we can just assume that someone is running the same version of every component because we don't support using older and newer versions together.

.github/ISSUE_TEMPLATE/bug_report.md

dekkers · 2022-12-22T15:06:14Z

.github/ISSUE_TEMPLATE/bug_report.md

+---
+
+_Please add `bug` and one or more of the following labels to your issue:_
+`frontend backend community dependencies`


What does community / dependencies label mean? Also frontend/backend doesn't always have the same meaning so I don't think they are good tags (some people see browser als frontend and server as backend, others see a UI service like rocky as frontend and the rest of the services as backend). We shouldn't ask people to tag things if we don't explain what the tags mean.

dekkers · 2022-12-22T15:09:07Z

docs/source/guidelines/logging.rst

+
+- `<https://tuhrig.de/my-logging-best-practices/>`_
+- `<https://web.archive.org/web/20201023044233/>`_
+- `<https://nerds.kueski.com/better-logging/>`_


These two links are broken

Darwinkel · 2022-12-29T12:30:07Z

Per JP's request, I've removed the logging document and moved the discussion here: #100

@dekkers is the revised bug report adequate?

Darwinkel added 3 commits December 16, 2022 15:25

Update repo README to include a link to the Sphinx docs

6322eaa

Add GitHub templates

375a18b

Update docs with logging guidelines

51bf726

Darwinkel self-assigned this Dec 16, 2022

Darwinkel and others added 2 commits December 16, 2022 15:56

Add line breaks and make a note about basing work off release tags

245bb05

Merge branch 'main' into feature/updated-docs

e3e722f

ammar92 previously approved these changes Dec 21, 2022

View reviewed changes

Darwinkel marked this pull request as ready for review December 22, 2022 12:22

Darwinkel requested a review from a team as a code owner December 22, 2022 12:22

Merge branch 'main' into feature/updated-docs

4619fbf

underdarknl previously approved these changes Dec 22, 2022

View reviewed changes

dekkers reviewed Dec 22, 2022

View reviewed changes

Darwinkel and others added 3 commits December 29, 2022 11:50

Merge branch 'main' into feature/updated-docs

dc57da9

Remove logging document

db99b7c

Process bug report feedback

f09d45a

Darwinkel dismissed stale reviews from underdarknl and ammar92 via f09d45a December 29, 2022 12:27

Fix trailing whitespace

34f3e6d

dekkers approved these changes Dec 29, 2022

View reviewed changes

dekkers changed the title ~~Update Sphinx docs with logging guidelines~~ Update Sphinx docs Dec 29, 2022

dekkers merged commit 0752827 into main Dec 29, 2022

dekkers deleted the feature/updated-docs branch December 29, 2022 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Sphinx docs #97

Update Sphinx docs #97

Darwinkel commented Dec 16, 2022 •

edited

Loading

dekkers Dec 22, 2022

jpbruinsslot Dec 27, 2022

dekkers Dec 22, 2022

jpbruinsslot Dec 27, 2022

dekkers Dec 22, 2022

jpbruinsslot Dec 27, 2022

dekkers Dec 22, 2022

jpbruinsslot Dec 27, 2022

dekkers Dec 22, 2022

jpbruinsslot Dec 27, 2022

dekkers Dec 22, 2022

dekkers Dec 22, 2022

dekkers Dec 22, 2022

dekkers Dec 22, 2022

Darwinkel commented Dec 29, 2022

		Some general guidelines for how to log within your applications:

		- Log after, not before. This communicates what operation was performed, and what the result was.

Update Sphinx docs #97

Update Sphinx docs #97

Conversation

Darwinkel commented Dec 16, 2022 • edited Loading

Changes

Issue ticket number and link

Proof

Checklist for author(s):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darwinkel commented Dec 29, 2022

Darwinkel commented Dec 16, 2022 •

edited

Loading