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

ZSS Logging Improvements #444

Open
wants to merge 17 commits into
base: v2.x/staging
Choose a base branch
from

Conversation

jordanfilteau1995
Copy link
Contributor

@jordanfilteau1995 jordanfilteau1995 commented Apr 29, 2024

Proposed changes

Changing the logging format to have useful information. I've added the following:

- Time Stamp
- TCB (Task Control Block) Address
- Thread ID
- Process ID
- File Name
- Function Name
- Line Number
- Severity Level

It looks like this:

2024-04-30 18:53:40.345 ZWESZ1:0x8FB2F8:0x37455E58:67241934 TS5873 CRITICAL (loggingtest.c:main:18) oh no!

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Testing

It should be non-breaking. If the new code is in, then the log messages done by ZSS will have the additional metadata by default.

Output from loggingtest.c.

2024-04-30 18:53:40.345 <ZWESZ1:0x8FB2F8:0x37455E58:67241934> TS5873 CRITICAL (loggingtest.c:main:18) Hello from zowelog2(). This log should have every field.
2024-04-30 18:53:40.345 <ZWESZ1:0x8FB2F8:0x37455E58:67241934> TS5873 INFO Hello from zowelog(). This log should lack fileLocation information.
2024-04-30 18:53:40.345 <ZWESZ1:0x8FB2F8:0x37455F20:67241934> TS5873 NA Hello from zowelog(). This log should lack level and fileLocation information.

This demonstrates support for backwards compatibility and native improvements to all current log statements, without changing code.

Further comments

Note: I think that we should move toward a logger that takes a message key and has messages stored in JSON or YAML with various metadata. This is something that I believe would be useful.

{ "key": "someZssError", "prefix": "ZSS", "number": 1, "severity": "I", "format": "This is a test message: %s." }

@jordanfilteau1995 jordanfilteau1995 changed the title Jordan logging improvements ZSS Logging Improvements Apr 29, 2024
Copy link
Member

@1000TurquoisePogs 1000TurquoisePogs left a comment

Choose a reason for hiding this comment

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

I love the idea of improving the logger but please do not invent a different syntax.

This is the format all of the servers except zis/zss are following:
zowe/zac#90

Please revise the PR to make zss match that.
( note, launcher already does as https://github.com/zowe/launcher/blob/v2.x/staging/src/main.c#L288 )

@jordanfilteau1995
Copy link
Contributor Author

@1000TurquoisePogs Gotcha! I've made changes to reflect the documentation you sent me.

c/logging.c Outdated Show resolved Hide resolved
@1000TurquoisePogs 1000TurquoisePogs dismissed their stale review May 1, 2024 14:29

changes addressed

h/logging.h Outdated Show resolved Hide resolved
Copy link
Contributor

@ifakhrutdinov ifakhrutdinov left a comment

Choose a reason for hiding this comment

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

Please see my comments.

And another note: I don't think you have addressed my concerns (in the issue) about the handler2 being potentially missing in a mixed level environment. Do you think we need to worry about that?

h/logging.h Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
c/logging.c Outdated Show resolved Hide resolved
@jordanfilteau1995
Copy link
Contributor Author

@ifakhrutdinov thanks so much for reviewing! I'll get to these :).

Signed-off-by: Jordan Filteau <[email protected]>
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.

Extend the logging facility to include additional information in messages
3 participants