-
Notifications
You must be signed in to change notification settings - Fork 123
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
Flogger automated migration from other logging APIs via Error Prone #71
Comments
Wow, thanks for looking at this. Inside Google there's a quite sophisticated (but not error-prone) mechanism for doing migrations, and while the code itself is a bit hard to open-source, it does solve a lot of common problems (we decided to not just migrate people's log statements, but to canonicalize them in the process since there were so cases of "bad" or at least "questionable" usage). e.g. Old style JDK log statements like: And common errors (e.g. bad escaping or wrong number of placeholders) would be detected and, in some cases, automatically fixed. I'm happy to share the sort of checks and transformations we're doing if it might help you. There are also some internal error-prone checks for Flogger correctness which we might be able to open-source if we haven't already (ensuring that once people have migrated, they conform to best practice). |
Would welcome any insight into checks / transformations - thanks! So far have managed to canonicalize various source APIs, such as:
and
and
into their canonical Flogger form, such as:
Current plan is to migrate (canonicalize) log statements, including transforming message format strings, leaving further refactorings / verifications to subsequent Flogger checks, such as:
...and likely others. It's likely that Google's Flogger checks overlap heavily here. Not finding any Flogger checks in open-source Error Prone (https://errorprone.info/bugpatterns); if those could be made open-source that would be great, no point in re-inventing them. |
Just FYI, escaping in printf is done with '%', so:
Looks wrong to me, I think it should be:
|
Thanks. Missing context: that example was from this SLF4J input:
...where SLF4J treats |
Ahh okay. It's just that as written:
would trigger a "too few parameters" error in our checks and the runtime code, since it expects 2 arguments for the 2 placeholders. Also, as regards existing checks, I think we do most of what you're suggesting already, so it's definitely not worth spending much time on that until I can get an answer about open-sourcing what we have. |
For awareness, working on Error Prone refactoring to migrate from other logging APIs:
https://github.com/cslee00/digitalascent-errorprone-logger
The text was updated successfully, but these errors were encountered: