-
Notifications
You must be signed in to change notification settings - Fork 203
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
[rqd] [cuegui] Add support for Loki for frame logs #1577
base: master
Are you sure you want to change the base?
[rqd] [cuegui] Add support for Loki for frame logs #1577
Conversation
…vided by ch.qos.logback:logback-classic and causing conflict
Add loki details to jobs in database using extra fields
# Conflicts: # cuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.java
Please remove the "Draft" status when this is ready for review |
# Conflicts: # cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java # cuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.java
public boolean lokiEnabled; | ||
public String lokiURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining what what they are and where they are being set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
public Boolean logLokiEnabled; | ||
public String logLokiURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
alter table job | ||
add b_loki_enabled bool; | ||
|
||
alter table job | ||
add str_loki_url varchar(256); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely sold on the idea of every job having these set. I agree runFrame is the best place to communicate this value from cuebot to rqd, but having to store this on the job table doesn't make too much sense to me.
prepareRqdRunFrame
on dispatch support could read the logpath directly from jobLogUtil, keeping the database out of the loop
# Loki | ||
log.loki.enabled=false | ||
log.loki.url=http://localhost/loki/api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please elaborate on the docs here. I would add:
- link to a tutorial on how to setup loki
- quick description of what happens when this feature is on
- what to expect if an invalid url is provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added small description. Will add general Loki documentaion on opencue.io
Should buffering be part of this PR? Or should it be part of a more generic system for buffering log writes? |
Link the Issue(s) this Pull Request is related to.
#1571
Summarize your change.
This adds the ability to use loki as the backend for frame logs in rqd. It also adds a new plugin/widget in cuegui to read the logs from the loki server.
This enables logs files to not be bound by a single namespace and also adds the potential to also store telemetry about the frame.
This is still on an experimental stage and any inputs are appreciated.
Screenshot of new widget :
Not yet implemented :