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

[WIP] JSON Log Files #70830

Closed
wants to merge 31 commits into from
Closed

[WIP] JSON Log Files #70830

wants to merge 31 commits into from

Conversation

ZephyrTFA
Copy link
Contributor

@ZephyrTFA ZephyrTFA commented Oct 27, 2022

About The Pull Request

// TODO ZEPHYRTFA

Log Viewer pictures

img
img
img
img
img
img

This is a heavy WIP, and simply being put up early to convince me to actually finish this, (and early review)

Why It's Good For The Game

// TODO ZEPHYRTFA

Changelog

🆑
admin: New log viewer, crawling logs has never been easier!
admin: Hey guess what, a brand new log system, no more crawling through files!
refactor: The entire logging system just about
/:cl:

@tgstation-server tgstation-server added Administration As generous gods, we have deigned to throw the jannies a bone Refactor Makes the code harder to read labels Oct 27, 2022
@bobbah
Copy link
Member

bobbah commented Oct 27, 2022

what are the memory and performance implications of this

@bobbah
Copy link
Member

bobbah commented Oct 27, 2022

also you will need to make sure that the logfiles are actually produced even if the server crashes; this is something that (while it is hard to ascertain from the current state of the pr) you may be about to forget about

under the current system logfile lines are written to disk as the round progresses, server crashes do not prevent us from seeing what was going on in the round

Copy link
Member

@san7890 san7890 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's WIP, but here's two thoughts I had

code/controllers/subsystem/logging.dm Outdated Show resolved Hide resolved
code/datums/logging/_log_entry.dm Outdated Show resolved Hide resolved
@dragomagol dragomagol self-assigned this Oct 27, 2022
@MrStonedOne
Copy link
Member

MrStonedOne commented Oct 28, 2022

CONFIG_GET(string/server) gets the URL + port, which in our case hides the real IP.

This does not work. none of the servers have it set ever except by vv for a round.

Also its a stupid setting that has side effects.

Cross_coms_name server_name, sql_name, are all fallback names that could be used. world.port how ever should be the main fallback, its how /tg/ used to differentiate servers and since byond has no default port, supporting the same port on multiple servers by the same community when the config(s) exists seems silly. world.address is unreliable and trying to read it can actually sleep in edge cases.

Adding in a config for public hostname/url is actually something we should do, and rename server to reconnect_uri because all server really does is just forces a byond redirect to the configured address on round end (like i'll vv server to basil's url when i take down sybil) instead of letting byond keep the connections open while pretending they are new connections in game code (the better option for server load reasons).

@tgstation-server tgstation-server added the UI We make the game less playable, but with round edges label Oct 29, 2022
Co-authored-by: Aleksej Komarov <[email protected]>
remove unused var
recover entries_by_key
@Mothblocks
Copy link
Member

BTW I don't know if oranges would disagree but something like this is never going to fully replace text logs, not as a matter of churn but as a fact that it would be a bad idea to impose too much work on something like logging.

This is going to be extremely useful still I just want to make sure we acknowledge that

{entries.map((entry) => (
<Flex.Item key={entry.key}>
<Stack.Item key={entry.key}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace this Stack.Item with a Box and have a similar effect (minus the gaps). The Stack itself can be removed.

onTextChange={(text) => setFilterText(text)}
onFlagsChange={(flags) => setFilterFlags(flags)}
/>
</Stack.Item>
)}
<LogCategoryBar
Copy link
Member

@stylemistake stylemistake Oct 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to wrap this in a Stack.Item with grow property (as I desribed it to you in Discord), and section (part of LogViewerContent) must have a "fill" property. Stack -> Stack.Item hierarchy is important to maintain. You also might want to split LogCategoryBar from LogViewerContent, and instead of nesting have them as individual Sections wrapped in individual Stack.Items.

@optimumtact
Copy link
Member

BTW I don't know if oranges would disagree but something like this is never going to fully replace text logs, not as a matter of churn but as a fact that it would be a bad idea to impose too much work on something like logging.

This is going to be extremely useful still I just want to make sure we acknowledge that

Then why the heck are you encouraging them to keep going?

Why would we have two logging systems

@ZephyrTFA
Copy link
Contributor Author

Well that was a lot of wasted work

@ZephyrTFA ZephyrTFA deleted the json_log branch October 31, 2022 02:07
@optimumtact
Copy link
Member

optimumtact commented Oct 31, 2022

you should PR it to a downstream

sorry that this happened btw, I never asked anyone to do my log overhaul because it was never a fully fleshed out idea. @ZephyrTFA who did you talk to that encouraged you to start this?

I always intended it to replace the entire existing logging system, yes it requires more developer overhead, but they can suck it up for server admins lives being easier.

@optimumtact
Copy link
Member

@ninjanomnom you have to decide if you want two logging systems or not, mothblocks and I cannot agree

@GoldenAlpharex
Copy link
Contributor

I think the little amount of developper overhead is worth it, even if it's mostly to enforce a certain format. We can make wrappers for stuff, I'm sure, with time it won't be much worse than it currently is, it'll just mean you'll have to think for more than two seconds about how you format your log entry.

@Mothblocks
Copy link
Member

Mothblocks commented Oct 31, 2022

Maintainers have historically never enforced adding logs to things outside of very obvious cases because even to the ones that are regular admins, it's not obvious what you'll need until you need it

If this was required you would see people add logs far less

I'm not saying it isn't worth it, I'm saying people won't do it without being asked or if they personally benefit from it

@GoldenAlpharex
Copy link
Contributor

I come back here after many months, bearing ninja's response to this, saying that he's fine with the JSON logging, so long as it is the only system, and not there alongside the old logging system.
image

@ZephyrTFA
Copy link
Contributor Author

I have a different backend already in place, this just means I can begin JSON'ification of existing logs

@ZephyrTFA
Copy link
Contributor Author

Which means my log viewer baby can become alive again, which is pog

@Mothblocks
Copy link
Member

You are never going to see people adding logs without being asked again

@optimumtact
Copy link
Member

you will have to make a new PR this one cannot be reopened

@ZephyrTFA
Copy link
Contributor Author

you will have to make a new PR this one cannot be reopened

I need to make a new PR anyway; since I already have a json logging back end in place from a previous PR that I want to make use of

@MrStonedOne
Copy link
Member

MrStonedOne commented Feb 12, 2023

I am still holding firm that logs have to be human readable descriptive lines in files first and data/machine formated whatever second.

If any json system can't produce log files that look like our current logs (descriptive lines in files that state what event happened in human terms) its not gonna pass my mark.

@optimumtact
Copy link
Member

they would be seperate logfiles are you high

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Administration As generous gods, we have deigned to throw the jannies a bone Refactor Makes the code harder to read UI We make the game less playable, but with round edges
Projects
None yet
Development

Successfully merging this pull request may close these issues.