Add support for logging to Loki (and others) #1401

lithorus · 2024-07-01T21:12:48Z

Summarize your change.

Have introduces a new module and class for RQD which deals with writing the process log file.

This is to abstract file writing which gives the ability to support non-file based logging (eg. loki, elasticsearch)

I have not been able to test the code changes on Windows and Mac

It depends on #1416 since the loki python module is not compaible with python 2.x

Still work-in-progress and comments on the implementation are welcome...

RQDLogger

A new python module (rqlogging) with the new class RQDLogger has been added to RQD to abstract any process logging. It's supposed to be a drop in replacement for a file object with some extra functions (like waiting for the file to be created). It also checks if it's using a "protocol" and will instantiate a sub-class for the appropriate protocol.
This makes it easy to centralize the logging logic and also adds the ability to extend with more logging backends.

How to use it

---- Cuebot configuraton ----

Cuebot provides the path to the root of logs, so this is where RQD is configured for logging.
It can either be configured using the environment variable CUE_FRAME_LOG_DIR or using the log.frame-log-root.default_os propterty in the config file.

This is also what's used to determine whether or not to use loki (at job submission time) instead of plain file logging. Only if the "protocol" is defined, it will not log straight to files. (however still using the wrapper)

To confgure for loki, a path like this can be used :
loki://internal/shows
loki being the protocol
internal is refering to the server instance
/shows is refering to prefixed path on the server

The server instance variable is not the actual address. but a reference to a server definition in RQD's config

---- RQD configuration ----

in rqd.conf :

[Loki]
internal=http://127.0.0.1:3100/loki

internal is the server reference name
http://127.0.0.1:3100/loki is the actual address used for submitting logs to the server

---- Cuegui configuration ----

in cuegui.yaml :

loki.servers:
  internal: http://127.0.0.1:3100/loki

internal is the server reference name
http://127.0.0.1:3100/loki is the actual address used for pulling logs from the server

When reading a log file in cuegui it looks like this :

---- Limitation ----
Currently it does not support multiple logs from the same frame in CueGui. (for non-file based logging)
Does not yet support authentication and "X-Scope-OrgID" is hardcoded to "1" (when using Loki)

…es with PySide6

…oggers and simplify prepend with timestamp

lithorus · 2024-07-01T21:16:13Z

I should also mention that I've replaced the pipe_to_file with a MUCH simpler method that "should" be cross-platform.

Replace wrapper and directly instance object

… python 3.9

# Conflicts: # requirements.txt

…on options for loki servers

lithorus · 2024-07-08T23:08:56Z

It should be noted that I cannot re-produce the error in the "Python Unit Tests (CY2023)" locally (in the docker container).

# Conflicts: # rqd/rqd/rqcore.py

Signed-off-by: Jimmy Christensen <[email protected]>

lithorus · 2024-07-16T21:44:40Z

I'm afraid I had to undo the changes from #1335 since this changes how things are logged. Hopefully it should be easier to re-implement again.

ramonfigueiredo · 2024-07-16T23:56:22Z

I'm afraid I had to undo the changes from #1335 since this changes how things are logged. Hopefully it should be easier to re-implement again.

Hi @lithorus

Ok, I see.

How does your solution with Loki handle non-ASCII characters in the logs?

What happens with RQD when logs contain characters like these: 영화, café?

Does RQD crash with non-ASCII characters in the logs?

Additionally, I think it would be beneficial to provide a logging solution using Loki that can be enabled or disabled. This way, we can revert to the previous default logging methods if Loki logs are disabled.

So for me, this current solution should be kept if the Loki logs are disabled.

…ting out log in RQLogger

lithorus · 2024-07-17T07:09:54Z

I'm afraid I had to undo the changes from #1335 since this changes how things are logged. Hopefully it should be easier to re-implement again.

Hi @lithorus

Ok, I see.

How does your solution with Loki handle non-ASCII characters in the logs?

What happens with RQD when logs contain characters like these: 영화, café?

Does RQD crash with non-ASCII characters in the logs?

Additionally, I think it would be beneficial to provide a logging solution using Loki that can be enabled or disabled. This way, we can revert to the previous default logging methods if Loki logs are disabled.

So for me, this current solution should be kept if the Loki logs are disabled.

You're right, it would crash if it got non-ascii characters. I tested it out with cat /usr/bin/ps in the render command and it would crash. I've now implemented a generic way to convert (although I settled for utf-8 instead).

I will wrote the details on how Loki is enabled/disabled in the summary of this PR.

ramonfigueiredo · 2024-07-17T17:54:19Z

I'm afraid I had to undo the changes from #1335 since this changes how things are logged. Hopefully it should be easier to re-implement again.

Hi @lithorus
Ok, I see.
How does your solution with Loki handle non-ASCII characters in the logs?
What happens with RQD when logs contain characters like these: 영화, café?
Does RQD crash with non-ASCII characters in the logs?
Additionally, I think it would be beneficial to provide a logging solution using Loki that can be enabled or disabled. This way, we can revert to the previous default logging methods if Loki logs are disabled.
So for me, this current solution should be kept if the Loki logs are disabled.

You're right, it would crash if it got non-ascii characters. I tested it out with cat /usr/bin/ps in the render command and it would crash. I've now implemented a generic way to convert (although I settled for utf-8 instead).

I will wrote the details on how Loki is enabled/disabled in the summary of this PR.

Great, thanks @lithorus !

lithorus · 2024-07-17T18:43:48Z

I'm afraid I had to undo the changes from #1335 since this changes how things are logged. Hopefully it should be easier to re-implement again.

Hi @lithorus
Ok, I see.
How does your solution with Loki handle non-ASCII characters in the logs?
What happens with RQD when logs contain characters like these: 영화, café?
Does RQD crash with non-ASCII characters in the logs?
Additionally, I think it would be beneficial to provide a logging solution using Loki that can be enabled or disabled. This way, we can revert to the previous default logging methods if Loki logs are disabled.
So for me, this current solution should be kept if the Loki logs are disabled.

You're right, it would crash if it got non-ascii characters. I tested it out with cat /usr/bin/ps in the render command and it would crash. I've now implemented a generic way to convert (although I settled for utf-8 instead).
I will wrote the details on how Loki is enabled/disabled in the summary of this PR.

Great, thanks @lithorus !

Have added some more details on it now.

DiegoTavares · 2024-07-18T21:07:40Z

rqd/rqd/rqcore.py

+        while True:
+            output = frameInfo.forkedCommand.stdout.readline()
+            if not output and frameInfo.forkedCommand.poll() is not None:
+                break
+            if output:
+                self.rqlog.write(output, prependTimestamp=rqd.rqconstants.RQD_PREPEND_TIMESTAMP)


This logic doesn't seem to have the same problems of the previous logic when RQD_PREPEND_TIMESTAMP=False (See #1426 ) but one think I'm not quite sure I understand is how stderr is being parsed.

The logic from pipe_to_file that got removed allows parsing both stdout and stderr into a single output without blocking on waiting on one of the pipes. Probably something similar will be required if we want both stdout and stderr content to be sent to Loki.

From what I remember both STDOUT and STDERR is being piped and thus end out the same way. I'm almost certain I tested exactly for that. Let me double check it.

Yes, both STDOUT and STDERR works (tested it with the file output, but should be the same for loki)

Btw, will it be ok to have the loki part still being beta/experimental? This PR changes alot more than just adding support for loki.

Now I remember how it was done. stderr is sent to stdout, of course there won't be any distinguish between the 2, which might be good to have in loki as seperate tags.

stderr=subprocess.STDOUT,

lithorus · 2024-07-22T20:07:29Z

@DiegoTavares I've created another PR with only the log abstraction and without the Loki part. There was still missing a few bits and pieces on that part before it was ready for production and would like to explore a few ideas on how to make it more modular.

DiegoTavares · 2024-07-24T20:06:12Z

@DiegoTavares I've created another PR with only the log abstraction and without the Loki part. There was still missing a few bits and pieces on that part before it was ready for production and would like to explore a few ideas on how to make it more modular.

Great, I have it on my list to be reviewed soon.

As this change adds a new feature, it will require changes to the documentation on opencue.io's Other Guides section. This website is maintained in this repo.

lithorus · 2024-07-31T19:33:01Z

Will re-do the implementation.

@ramonfigueiredo

**Link the Issue(s) this Pull Request is related to.** #1401 #1427 **Summarize your change.** This abstracts the reading of logs and writing log files. This makes it simpler to do things like supporting other logging types and conversions (eg. make sure things are ASCII) in a single class across all platforms. This is a smaller version of #1401 with just the file logging abstracted. It also implements the changes from #1427 Co-authored by @ramonfigueiredo and @lithorus --------- Signed-off-by: Jimmy Christensen <[email protected]> Co-authored-by: Ramon Figueiredo <[email protected]>

@ramonfigueiredo

…demySoftwareFoundation#1429) **Link the Issue(s) this Pull Request is related to.** AcademySoftwareFoundation#1401 AcademySoftwareFoundation#1427 **Summarize your change.** This abstracts the reading of logs and writing log files. This makes it simpler to do things like supporting other logging types and conversions (eg. make sure things are ASCII) in a single class across all platforms. This is a smaller version of AcademySoftwareFoundation#1401 with just the file logging abstracted. It also implements the changes from AcademySoftwareFoundation#1427 Co-authored by @ramonfigueiredo and @lithorus --------- Signed-off-by: Jimmy Christensen <[email protected]> Co-authored-by: Ramon Figueiredo <[email protected]>

@ramonfigueiredo

…demySoftwareFoundation#1429) **Link the Issue(s) this Pull Request is related to.** AcademySoftwareFoundation#1401 AcademySoftwareFoundation#1427 **Summarize your change.** This abstracts the reading of logs and writing log files. This makes it simpler to do things like supporting other logging types and conversions (eg. make sure things are ASCII) in a single class across all platforms. This is a smaller version of AcademySoftwareFoundation#1401 with just the file logging abstracted. It also implements the changes from AcademySoftwareFoundation#1427 Co-authored by @ramonfigueiredo and @lithorus --------- Signed-off-by: Jimmy Christensen <[email protected]> Co-authored-by: Ramon Figueiredo <[email protected]>

lithorus and others added 14 commits June 15, 2024 18:32

Switch to PySide6

c764d48

Update qtpy to 2.4.1

a407909

Update future and six for better support of python 3.11+

b9c4bcf

Specific requirements for different versions of python

44b7a30

Update PySide2/PySide6 support matrix

0229720

Update PySide2/PySide6 support matrix

7f7d7e6

Update PySide2/PySide6 support matrix

5c6224e

Remove override scrollContentsBy which seems to create threading issu…

c45f6ca

…es with PySide6

Merge remote-tracking branch 'origin/master' into pyside6

b19a80d

change PySide2 imports to qtpy

52adb10

Merge remote-tracking branch 'origin/master' into pyside6

2f981ef

Add rqlogging.py with RQDLogger class to simplify creating abstract l…

2d480b7

…oggers and simplify prepend with timestamp

Use new logging method for Windows and Mac

0afeab7

re-enable log.debug(runFrame)

9005418

lithorus added 14 commits July 1, 2024 23:27

Fix pylint errors

a5ecbff

Use logging level from config file

53fc1ab

Add support for loki using loki:// protocol prefix

b790749

Replace wrapper and directly instance object

Move directory creation and file rotation into the RQDLogger class

cb68bb5

Add specific version of urllib3 which supports old ssl module used by…

09bc4b2

… python 3.9

Fix linting issues

bbbaab3

Get specific logger per file

75c222f

Only use single argument for print

b58d41d

Merge remote-tracking branch 'origin/master' into loki-logger

438745d

# Conflicts: # requirements.txt

Add environment variable to force running node16 on old distros

c0ab4a2

Add LogViewPlugin.LokiReader and rqlogging.LokiLogger and configurati…

0d7fe6d

…on options for loki servers

MAke compatible with older python versions

1e35a2e

Remove debug print

bbf880d

Fix pylint errors

f3f891e

lithorus mentioned this pull request Jul 16, 2024

[rqd] Fix non ASCII chars #1335

Merged

lithorus and others added 2 commits July 16, 2024 23:13

Merge remote-tracking branch 'origin/master' into loki-logger

1543bdb

# Conflicts: # rqd/rqd/rqcore.py

Merge branch 'master' into loki-logger

fc85a9f

Signed-off-by: Jimmy Christensen <[email protected]>

Process forkedCommand output as bytes and convert to utf-8 before wri…

3aba2e0

…ting out log in RQLogger

lithorus added 2 commits July 17, 2024 20:56

Merge remote-tracking branch 'origin/master' into loki-logger

597474b

Increase default limit to 1000

cf0b2d1

DiegoTavares reviewed Jul 18, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into loki-logger

6de76dd

lithorus mentioned this pull request Jul 22, 2024

Abstract logging into seperate classes (RQDLogger and LogReader) #1429

Merged

lithorus closed this Jul 31, 2024

lithorus deleted the loki-logger branch September 19, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for logging to Loki (and others) #1401

Add support for logging to Loki (and others) #1401

lithorus commented Jul 1, 2024 •

edited

Loading

lithorus commented Jul 1, 2024

lithorus commented Jul 8, 2024

lithorus commented Jul 16, 2024

ramonfigueiredo commented Jul 16, 2024

lithorus commented Jul 17, 2024 •

edited

Loading

ramonfigueiredo commented Jul 17, 2024

lithorus commented Jul 17, 2024

DiegoTavares Jul 18, 2024

lithorus Jul 18, 2024 •

edited

Loading

lithorus Jul 18, 2024

lithorus Jul 18, 2024

lithorus Jul 18, 2024

lithorus commented Jul 22, 2024

DiegoTavares commented Jul 24, 2024

lithorus commented Jul 31, 2024

Add support for logging to Loki (and others) #1401

Add support for logging to Loki (and others) #1401

Conversation

lithorus commented Jul 1, 2024 • edited Loading

Summarize your change.

RQDLogger

How to use it

lithorus commented Jul 1, 2024

lithorus commented Jul 8, 2024

lithorus commented Jul 16, 2024

ramonfigueiredo commented Jul 16, 2024

lithorus commented Jul 17, 2024 • edited Loading

ramonfigueiredo commented Jul 17, 2024

lithorus commented Jul 17, 2024

DiegoTavares Jul 18, 2024

Choose a reason for hiding this comment

lithorus Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

lithorus Jul 18, 2024

Choose a reason for hiding this comment

lithorus Jul 18, 2024

Choose a reason for hiding this comment

lithorus Jul 18, 2024

Choose a reason for hiding this comment

lithorus commented Jul 22, 2024

DiegoTavares commented Jul 24, 2024

lithorus commented Jul 31, 2024

lithorus commented Jul 1, 2024 •

edited

Loading

lithorus commented Jul 17, 2024 •

edited

Loading

lithorus Jul 18, 2024 •

edited

Loading