crs-tools / tracker

CRS Ticket Tracker
Apache License 2.0
18 stars 11 forks source link

disable logging for progress updates #220

Open Garionion opened 5 years ago

Garionion commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

a-tze commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Garionion commented 3 years ago

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)