Disable extra logging by default #5686
Conversation
|
No this is not okay. Component logging is to be auto-enabled when debugging is enabled, and CEC is to be auto-enabled as component. I've objected against this change 10 times now, also when the original PR was done to turn this into a component, but apparently everyone who doesn't do anything with CEC knows this better than me, so I'm done with it. If this goes in, then I'm just going to make libCEC more verbose. A lot more verbose. |
|
So basically what you're saying is that there are 10 component logging classes (CEC excluded) which are all disabled by default which makes the debugging life of all developers easier except for the one that is responsible for that component and that every maintainer of these 10 component logging classes can live with that but that CEC is completely different than all of them. Nobody ever said that they know CEC better than you and nobody questions that you need these log messages but the same is true for every other component logging class and its maintainer and they can all live with it. From the other PR it sounded like (but maybe I misunderstood) there's one particular log message that is very important (the one logging each keypress) so maybe just logthat one log message as a normal debug message (independent of the component) and keep all the others in the CEC specific component logging? |
|
I've already explained it in the previous PR (or the one before that actually iirc) why I'm against having people switch 3 options to get the logging that I need. In short: enabling debugging should enable debugging, and asking users, who generally only know how to plug in a USB cable, to flip 3 options (which may be translated, hidden depending on settings level, or placed elsewhere by a skin) may be possible, but is indeed too much. Perhaps people who play with json-rpc are a bit more tech savvy than CEC users... Another big difference compared to json-rpc is that your stuff doesn't depend on what the vendor of your TV, AVR or whatever other device is connected to HDMI. Each of these can (and in the past, often did) break CEC completely, just because some vendor interpreted the spec differently or because some vendor does nasty stuff to break things when combined with a device from another vendor. The init sequence and switching inputs is more important for logging than the keypress logging (though that tells me that things actually work). The reason why I compared it to keypress logging is that the main reason for turning it into a component in the first place, was the amount of logging that libCEC can do (again, the amount of logging differs per combination of devices, from only logging key presses, like most devices do, to logging ~20 lines every 10 seconds like only 2 vendors do). Keypress logging isn't stuck in a component either. Neither are a lot of other things, that can do a lot more spamming, and I don't care for any of that logging either. When the original PR was done to turn CEC into a component, I objected against it being disabled by default, and I have objected against this change multiple times now, and the only reason for this PR is that you're complaining about me treating CEC in a special way (sure, I do, I'm the one who has to fix these), but it was made special when it got turned into a component in the first place. It wasn't a component in Gotham and it is in Helix, and having no useful logging when people enable debugging is a regression to me that must be fixed. If you turn all other logging that xbmc does into a component, then it's no longer special perhaps, but it was made special in Helix and that needs to be undone or handled in such a way that it doesn't bother users (so enabling logging enables component logging too). Either that, or the solution that's in master now (enabling the CEC component and by default in settings), or I revert my support for turning it into a component and will revert the changes that introduced it. And if you won't have that, then I'm really going to make libCEC more verbose so I get the logging I need and have you jump through hoops to get it back out again. |
|
The main problem IMO is at a bigger level on how people interpret the option activate component logging. For CEC it's correctly implemented as the way I see it : This option is an additional thing over the debug settings. Meaning a component to log needs to have the debug option on and the option enable component logging on and it's specific line checked (Meaning 3 click that I agree is too much). For other like UPnP, component logging is an option by itself and activating it allows component to log even if debug log is off. Since the option as I reported earlier can be understood differently by users and devs it means the ways it is is not correct. For me the only sane way would be : Remove the useless enable component logging option. Any maintainer of a component can disable other component on it's dev machine and filtering logs that have everything in it is as easy as a simple grep. But keeping 3 options with different interpretation will only generate more troubles like this one. |
|
I agree, and that was the suggestion in the previous PR too, but then people complained that some of these components really spew a lot when enabled by default, so this solution (without removing the enable/disable switch for component logging) got reverted again. |
|
Well I was one of the complaining guy :) But only because components does logs tons of things even with debug turned off, and this is not acceptable to have by default a Kodi that logs 10 to 20 MB per day without doing anything ... But I do not care to have big logs when debug is turned off, as a dev, the more logs the better for me. That's why the implementation I propose handle this properly and avoid problems. (With of course full debug on until settings loading to catch startup errors ;) ) |
|
Keep in mind that all debug builds (meaning we compile it debug flags) has automagically turned debug logging on. Might be that that was the issue you saw. Since beta2 - beta builds are release builds where the debug log setting only determines if debug logging is on or off ... just thinking out loud here... |
|
Yeah, which makes it even nicer, the reason for this PR is that other components spew and CEC isn't to be treated differently from those other components when it comes to this option. |
|
@Memphiz that may be the cause too indeed. why isn't debug logging controlled by the option in debug mode anyway like in release? |
|
Well yes I suppose this was the major problem leading to all this noise. But I too wonder why an option in the GUI is not honored depending on how it's build ? This is counter intuitive for end users. And if it was the reason then remove the unneeded enable component switch (as it's can still be interpreted differently ) and leave all component to activated by default as previously (And of course do not publish build even alpha in debug mode or let the option control the damn thing) |
|
Here is an actual of example of someone telling me: "I have audio issues, Someone might guess what produces those "issues"? :-) Edit: To clarify I don't want to blame CEC, not at all. Everything that is logging by default is killing our mainloop and is blocking other important operations. Especially if we log to slow "usb like" media. |
|
@opdenkamp i don't know - way before my time |
|
@fritsch those aren't even debug messages from libCEC, but genuine errors because CEC support for that device is broken. You best disable CEC on that one until it's been fixed. Component logging or debug logging being disabled doesn't change anything about that. As for logging blocking other operations: that's why you can disable the CEC logging component with the current code. |
I just wanted to say that we should decide wise what to log when when we want to debug specific things. But thanks for that advice, I told the user to disable it completely. In short: If someone has a low power platform and wants to debug audio problems, we cannot tell him: Enable debuglogging, cause that way all of the components would spam like hell and produce even more sever issues than without the debug logging. |
|
In this log of yours, if CEC would be causing your audio issue, then pressing any key on the remote or keyboard, any announcement within xbmc, anything grabbing images from the net, etc, would all be causing the very same audio issue. |
|
Exactly, this is what I told. |
|
My thoughts is that we should have followed other logging libraries instead But @opdenkamp I agree that debug internal shouldn't be more important than Turning debug on to turn on all components is a patch I'd probably approve
|
|
What exactly doesn't make sense about having CEC logging turned on when debug logging gets enabled, while I have explained it multiple times already? What justifies the change from Gotham, where enabling debugging enabled CEC logging too, to the current situation. And "don't treat CEC different from other components" is an invalid argument, as that's exactly what got changed: CEC is a component now. Improving it for Helix+1 is a good idea, but that'll be Helix+1, and I want it working correctly in this release too. Which means not merging this PR or fixing it otherwise. |
|
The design is that debug is off by default and components are off by
|
|
Did you even read what I just said? CEC wasn't a component in Gotham. It's been turned into a component in Helix. |
|
So I'll be happy to revert it back to how it was, so I don't "break" the component logging design by having one of them enabled by default. |
|
Yes I read that. And that's not something I care about. I think it's a step back however but that's outside this PR.
|
|
Yes, I definitely agree that it's a step back to remove it, but the design of component logging with everything off by default is not something I care about either. I don't see any reason for changing things for end users because that's how the design is, when it can be fixed quite easily by just enabling the component by default for CEC. That way nothing changes for end users and devs can still (ask to) disable a component that they're not interested in, or that possibly affects test results. What reason is there for not having this option set by default for CEC other than "that's against the design". Whatever that may be, because it's just a default value for a setting, and doesn't touch any design. |
|
All other components are in that same problem. So its nothing cec specific Tbh I really dislike that we have logging options in GUI at all. But that's
|
|
@topfs2 If one day Kodi is perfect why not, but until them end users needs an easy way to turn debugging so people like me that do support for thousands of users can see Kodi problem and report / fix them ;) |
|
But unfortunately they do, unless we introduce some other way to do logging without filtering. Which is out of scope for Helix. You keep saying "other components", but that's the issue: CEC was turned into a component, and because of that, you are now saying that it's logging should be filtered out by default when debugging is enabled. That's chicken & egg. I don't want CEC to not use component logging, as I can see perfectly well that this is useful, and I agree that we should turn more things into components, so they can be filtered out too. But just changing the default in the way logging works because it's a component now is just not right. We'll complicate things for users, in a component that's known to break when vendors put some new model on the market that does things differently again. I can't see any valid reason to require some logging to be disabled by default once it's turned into a component. Say we turn our codecs logging into a component some day, do we then disable their logging by default too? Or VAAPI? Or audio? Lots of things should be turned into using component logging some day, but they really shouldn't all be disabled by default, unless we come up with some more user friendly solution to change these options and still get useful logging. That won't happen before we release Helix though, so until we've come up with something better, we should keep things the way they are now. |
|
This is exactly why I started the forum thread. I agree its terrible. I
|
Debug should definitely be off by default, but please explain why CEC logging should be turned off by default now that it's using component logging. Why do we have to make things more complex for end users before we come up with a better solution? These things are affected by lots of things, like translations, skinning, settings level, etc. It's not as easy as just saying "go here and click that", and unfortunately not all our users speak english very well or know their way around in our settings dialogs. |
|
And I'll look at the forum thread later, want to get some other stuff finished first ;) |
|
Why should json RPC be off. Why should upnp be off etc. Basically this is outside my control. What ever I do I'll piss off someone.
|
|
You're talking about the primary input of a lot of users here. Which json and upnp are not. But that's beside the point. You didn't answer my question, and keep repeating that it should be handled in the same way for everything. |
|
Well JSON is for a very large pool you know :) Way way more than CEC... Yatse is near 1 million user, official remote more, and not even talking about iOS remotes... CEC is supported only on a limited set of devices and needs compatible receiver, Tv, .... So from my point of view JSON > CEC ;) And from statistic / user base point of view too. So I do not even see the point of the CEC discussion, I do support for thousands of users every day, most do not speak English and yet I do not complain to have to explain them to turn 3 options on, even if I also find this stupid and think as everybody that it should be done better but until then well no choice :) |
|
I'm not so sure about your numbers, can you show them for CEC? :) Most users will want to use a physical remote rather than a virtual one I think, whether it's CEC or something else I don't know, but I know we've got plenty of RPi users that use CEC as their primary input source. But I don't see the point of making it more complex for those users that don't speak english or don't know their way around settings, and I haven't heard any reason for it yet. |
|
Yes I'm repeating. You are doing the same. I did not choose to do it as And as an RM this is the decision I'm taking. I don't want to waste more
|
|
Also I think we can take away the end user from this discussion. Logs are
|
|
Yes, I keep repeating that you have not answered my question indeed. If the team (without me in this case) has decided that logging should be turned off once it's turned into a component, then that is wrong, unless someone can name a very good reason for this requirement. We don't have two, we have at least three, in a list that can be translated, hidden, etc. Debug is debug in about every language I guess, so easy to find, and this makes it harder without any good reason, so yes, I complain about this. |
|
And, while logs are indeed a development tool, I don't have every combination of vendors and devices sitting next to me, so in order to find out why something doesn't work, I really need to get these logs from users. |
|
Yes this is why I created the logging thread. I think its wrong as well.
|
|
Yes I agree, so why the hard requirement on it being disabled by default for Helix, and make things more annoying for everyone involved? That's the thing I keep repeating indeed, and I haven't heard a single reason for this yet. If we can't have a single option turned on by default without any good reason for not allowing this, then I revert my support for the original PR that turned CEC into a component until we come up with something better. Even though that's a step back, as you already pointed out. |
|
You want my analytics that shows now 425 000 active monthly users ? (With BTW 60% being rpi users, since rpi Interface is too slow and using a remote with caching and lot's of tool give users a way better experience ...) I do not have CEC stats but I do know well my users ;) Please show me your stats ? But this is not the point here at all. And you keep repeating the same things that every body does agree ... |
|
@Tolriq yeah the other is internal only for now, I will probably take it @opdenkamp the decision is to merge this. The end. You have not convinced
|
|
Have it your way then, I'm going to make every future version of libCEC more spammy. |
Disable extra logging by default
In contrast to #5670 @opdenkamp not sure if you wanted to revert more from your original PR or if this is enough? Seemed like the rest was proper afaict.