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] Add logging support #31333

Closed
wants to merge 2 commits into from
Closed

Conversation

mitchcurtis
Copy link
Contributor

@mitchcurtis mitchcurtis commented Aug 12, 2019

This patch adds a debug_log() function (log() was taken; the name is up for discussion) to GDScript. The goals are to:

  • Allow logging that is only executed in debug builds.
  • Allow logging to different categories, which can be enabled/disabled in e.g. project settings.
  • Allow specifying a message pattern (e.g. in project settings) to enable things like timestamps to be included in the logged messages (see e.g. https://doc.qt.io/qt-5/qtglobal.html#qSetMessagePattern).
  • Allow logging to either the console or a file.

Closes #27958.

@mitchcurtis
Copy link
Contributor Author

mitchcurtis commented Aug 12, 2019

Opening this early so I can get early feedback.

What do you think about the goals of this patch?

I'm also curious to what extent (if any) this should use the existing Logger API internally.

@fire
Copy link
Member

fire commented Aug 12, 2019

You may be able to use and improve on this function for timestamps. get_iso_date_time

@Calinou
Copy link
Member

Calinou commented Aug 12, 2019

As a stretch goal, maybe we could have a setting to output all logging messages as JSON. This would be useful to feed a dedicated server's console output to a log aggregation tool such as Loki.

@mitchcurtis
Copy link
Contributor Author

As a stretch goal, maybe we could have a setting to output all logging messages as JSON. This would be useful to feed a dedicated server's console output to a log aggregation tool such as Loki.

Sounds fairly straightforward, assuming there's already a way to write JSON to a text file?

@Calinou
Copy link
Member

Calinou commented Aug 12, 2019

@mitchcurtis You would just need to print the JSON to standard output (one JSON object per line, that is). Writing to a file should be handled by writing that standard output to a file, as is already done by the Logging > File Logging > Enable File Logging project setting.

@Chaosus Chaosus changed the title WIP: Add logging support [WIP] Add logging support Aug 13, 2019
@Chaosus Chaosus added this to the 3.2 milestone Aug 13, 2019
@mitchcurtis mitchcurtis requested a review from a team as a code owner August 13, 2019 20:04
@mitchcurtis
Copy link
Contributor Author

mitchcurtis commented Aug 13, 2019

Is there a way I can suppress the notifications for reviewers until I'm ready? I still have a lot of work to do... perhaps I should have just kept this as a branch on my fork. :x

Edit: there are drafts, but you can't convert a regular pull request to a draft, only the other way around: https://github.saobby.my.eu.orgmunity/t5/How-to-use-Git-and-GitHub/Feature-Request-Switch-from-ready-to-draft-in-pull-requests/td-p/19107/page/2

The goal for logging in GDScript is to have as minimal overhead as
possible in debug builds. These new functions only execute their arguments
(expressions) passed to them if Godot was built in debug.
@mitchcurtis
Copy link
Contributor Author

@bojidar-bg do you know why I'd be getting this warning:

W 0:00:00:0447 Standalone expression (the line has no effect).

Scene.gd:5

for this:

extends Node2D

func _init():
    log_debug("MyCategory", "doing some stuff involving MyCategory")

I can see logging output in Creator's output panel, but not in Godot's.

}

if (args.size() != 2) {
_set_error("debug_log expects two arguments: category and message");
Copy link
Member

Choose a reason for hiding this comment

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

Remember to update the messages to remove references to debug_log and instead refer to the correct logging function 🙂

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Oct 8, 2019
@aaronfranke aaronfranke marked this pull request as draft April 9, 2020 00:33
@aaronfranke
Copy link
Member

@mitchcurtis Is this still desired? If so, it needs to be rebased on the latest master branch. Also, the above feedback should be addressed. Also, since this PR is a feature proposal, you should consider opening a proposal which explains example use cases and how this approach will solve the problem.

Otherwise, abandoned pull requests will be closed in the future as announced here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add built-in logging classes / capabilities
6 participants