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

disable logging for progress updates #220

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Garionion
Copy link

@Garionion Garionion commented Nov 25, 2019

ffmpeg has an option -progress where you can put an url, ffmpeg pushes approximately every second an update to. it is also possible to put a filename there an ffmpeg just writes the values into this file. I've build a little programm which sends some of these values to the tracker. But because of the short-lived nature of the values it would be unnecessary more stress on the database if there would be log entries created for those progress updates.

@a-tze
Copy link
Collaborator

a-tze commented Nov 25, 2019

Once there was special support for progress here:

https://github.com/crs-tools/tracker/blob/master/src/Application/Controller/XMLRPC/Handler.php#L522

Maybe you ressurrect this feature. Progress should not be stored as a regular ticket property, and the "progress" attribute of a ticket (not a property!) is calculated by the tracker, but not meant to be set directly by SetTicketProperties.

@Garionion
Copy link
Author

just to be clear, the things i would like to (optionally) send to the tracker are:
Progress.Bitrate=293300
Progress.Dup_frames=0
Progress.Frame=47
Progress.Out_time_ms=2210000
Progress.mFPS=9930
Progress.millispeed=467

I'm not quite shure how I would send those properties to the ping function

@a-tze
Copy link
Collaborator

a-tze commented Nov 25, 2019

Ok... if you really need this kind of information in the tracker itself, then maybe an optional parameter for the SetTicketProperties method that disables logging all together?

@Garionion
Copy link
Author

where should this parameter be set?
and: what if one sends non-progress properties together with progress properties?

@a-tze
Copy link
Collaborator

a-tze commented Nov 27, 2019

Without knowing the whole use case, I really think those things belong into a MQTT broker or similar.

However, my personal suggestion that might serve other purposes as well is to change this function declaration

https://github.com/crs-tools/tracker/blob/master/src/Application/Controller/XMLRPC/Handler.php#L594

to

public function setTicketProperties($ticket_id, array $properties, bool $logChanges = true)

but I'm not sure yet if this kind of declaration plays well with PHP and works with XML-RPC (because of the array before it). And I'm not the maintainer of the XMLRPC code ;)

@pegro
Copy link
Member

pegro commented Nov 27, 2019

Changing that method would be possible, but I don't like it.
I don't want to allow a malicious API client being able to change importent properties like Record.Cutin without creating a LogEntry.

I'm okay with the patch in general, maybe the filtered prefix might be something for a configuration variable.

But the general discussion, where to put processing information is still a valid one. It very much depends on who is going to use it. If the tracker wants to draw some cute encoding progress bars and display some nice ETA countdowns, then something like properties might be the right thing to do.
If more timeseries like data is interessting, to render plots of average encoding speeds for each encoder etc., then maybe an external storage is more appropriate.

@jjeising
Copy link
Member

I would be interested in these values in form of a new API call. We may even want to store them separately (ticket_progress_properties?), but always update in place (no historical / time series data). Maybe this table could track a running average if numeric data is detected.

@pegro
Copy link
Member

pegro commented Dec 2, 2019

Mh, not sure that introducing all that API, tables and model code only to avoid that if clause is worth it?!
What would be the conceptual difference between those two types of properties apart from "(not) being logged"?

@jjeising
Copy link
Member

jjeising commented Dec 2, 2019

I'm also not sure :)

What would be the conceptual difference between those two types of properties apart from "(not) being logged"?

Average calculation (this could also be done on the client), different presentation for the user, maybe auto updating when viewed? Feels like we should differentiate between properties (stable, tracked) and point in time values.

If standardized these values could also provide a guidance for a crsd work distributor in the future (e.g. encoding duration, average FPS).

@pegro
Copy link
Member

pegro commented Dec 2, 2019

I assume those "average" numbers won't change much during an encoding. Using the latest update on an encoding fps property should already give you a good overview how an encoder performs (using a query joining tbl_log "WHERE to_state = 'encoded'")?!

The timeseries stuff should only be added, if there is an actual use-case for these numbers during encoding.

The auto-update idea is good, but that could be done with the current property system and adding that filter preventing log entries getting generated, like "volatile properties", no?

@jjeising
Copy link
Member

jjeising commented Dec 2, 2019

The auto-update idea is good, but that could be done with the current property system and adding that filter preventing log entries getting generated, like "volatile properties", no?

I think we could add a new API method, see how it is used and move from there. setVolatileProperties? Would be more general than an arbitrary and undocumented prefix, I think.

@pegro
Copy link
Member

pegro commented Dec 3, 2019

You mean setVolatileTicketProperties? We have properties for tickets and projects.
But we would never merge them with the existing properties, right?
So that changing the effective Record.Cutin propety still gets logged.

@jjeising
Copy link
Member

jjeising commented Dec 3, 2019

But we would never merge them with the existing properties, right?

I'm not sure we can guarantee that, given we would store these properties in the same table.

But the client would use setTicketProperties for Record.Cutin and setVolatileTicketProperties for Encoding.Progress.Bitrate and Encoding.Progress.DroppedFrames (maybe we could even drop .Progress).

@pegro
Copy link
Member

pegro commented Dec 17, 2019

Like I said, I don't want to allow anybody changing important properties without logging.
I know the tracker doesn't know or care what is important, but I just feel bad about just hoping nobody misuses the API (accidentally).

@a-tze
Copy link
Collaborator

a-tze commented Dec 17, 2019

Therefore I suggest it not being called properties at all, but give it a different name, store it somewhere else and then it's by definition not important.

@a-tze
Copy link
Collaborator

a-tze commented Nov 24, 2020

@Garionion maybe this is better solved by using something like mqtt?

@Garionion
Copy link
Author

Isn't MQTT for transporting Messages? I'm not really sure what the point is to use mqtt instead of xmlrpc. (Also: if we are going to bypass the tracker api, and therefore likely bypass the tracker database too, i would also like to have timeseries data to better calculate ETA)

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.

4 participants