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

Log should not be i18n #2477

Closed
rochacbruno opened this issue Apr 4, 2022 · 10 comments · Fixed by #2577
Closed

Log should not be i18n #2477

rochacbruno opened this issue Apr 4, 2022 · 10 comments · Fixed by #2577
Assignees
Labels

Comments

@rochacbruno
Copy link
Member

rochacbruno commented Apr 4, 2022

Currently logs are being translated (passed by gettext _) and that causes log messages to be internationalizable and that is a problem.

Statement

The audience for logs are developers, system administrators, auditors, testers and as the main project documentation + code are in English we expect logs to be consistent.

Logging analysis tools would expect log levels (ERROR, CRITICAL, DEBUG, INFO) etc to be in english and there are some tools that might do some deeper linguistic analysis on logs.

Bad scenario

Assuming a system that is installed for a Brazilian company and the whole system messages on the front-ends (API, UI, CLI) are well translated, and that is expected!

Now on that system there is a problem and the user open an issue, the developer or tester asks the user to upload the latest log file and the developers, testers, administrators are going to get messages like:

[ERRO CRITICO]  Não foi possível efetuar a operação desejada, a entidade requisitada não foi encontrada.

Or if the system is in chinese

[严重错误] 无法执行所需的操作,未找到请求的实体。

As we don't have a table of standard error codes, it will be very difficult and costly for support to troubleshoot the issue.

It is better to keep in English as it is the standard lang for I.T support.

[CRITICAL]  The desired operation could not be performed, the requested entity was not found.

Conclusion

Only front-ends must be translated.

@bmbouter
Copy link
Member

bmbouter commented Apr 4, 2022

Thank you for bringing this up. I'm in favor of this change, and currently many of Pulp's log strings are wrapped with gettext. This would be something each plugin would also need to do. Maybe each mini team could make an issue to fix their log statements?

@dralley
Copy link
Contributor

dralley commented Apr 4, 2022

I'm also in favor of this, there have been times where gettext was causing an 8% overhead on sync tasks due to some hot log events.

@rochacbruno
Copy link
Member Author

If there is a need to translate logs, that probably would be a task for the log handler to perform this lazily.

On code we just emit the raw log.

@ipanova
Copy link
Member

ipanova commented Apr 4, 2022

+1 to this change too

@bmbouter bmbouter changed the title [suggestion] Log should not be i18n Log should not be i18n Apr 12, 2022
@bmbouter bmbouter self-assigned this Apr 12, 2022
@bmbouter
Copy link
Member

With this issue I plan to do two things:

  1. Un-i8ln the log statements throughout pulpcore
  2. Add a section in the plugin writer docs on i8ln outlining that we expect i8ln on all user-facing strings and we do not want the logs to be i8ln.

@bmbouter
Copy link
Member

In looking at fixing this, the log statements are very clear, but the error messages which produce tracebacks are difficult. What do I do in these 3 cases?

  • The error is raised in the tasking system?
  • The error is raised in an view or viewset?
  • The error is raised in both cases? There are some codepaths that do that.

@lubosmj
Copy link
Member

lubosmj commented Apr 21, 2022

Errors raised in views and viewsets are usually handled by the rest framework (https://www.django-rest-framework.org/api-guide/exceptions/). I would get rid of i18ln only for errors that result in an HTTP 500 response.

Regarding the tasking system, I believe that once the user's data successfully pass through the validation provided by serializers, we are safe to state that there was no user fault. Everything that goes beyond the stage of the Pulp API, should not be translated. If an error occurs during the runtime of a task, then there might be a problem in Pulp that needs attention from an administrator. Thus, no i18ln is required in this case. Here is what I did for pulp_container: https://github.com/pulp/pulp_container/pull/708/files.

However, there might be a scenario where a user wants to re-sync her repository. If the remote will not be accessible, the corresponding task will fail during the runtime. Here, we would like to provide a translated error message in the task's details. So, the real question is whether we should differentiate between particular types of task errors or not. 🤷‍♂️

@mdellweg
Copy link
Member

So, the real question is whether we should differentiate between particular types of task errors or not. man_shrugging

In general this is a great Idea. To me for example it is very annoying that URL not found (a clear misconfiguration of the remote by the user) produces half a kilometer of stacktrace both in the logs as well as in the task report.

@lubosmj
Copy link
Member

lubosmj commented Apr 21, 2022

Exactly! Some of the errors are raised in a very inappropriate place. I suggest first removing i18ln from all tasks. Once we move all the validation to the right place (e.g., serializers), then we may proceed with adding the support for translations.

@bmbouter
Copy link
Member

Thanks for all the discussion. There are hundreds of instances to look through so for time/benefit reasons I'm going to focus on removing the log i8ln in this PR and also document that as the official policy. I believe of the i8ln string used in exception messages those exclusive to tasks will be the minority so for me it's not worth it. I'll make that PR now.

bmbouter added a commit to bmbouter/pulpcore that referenced this issue Apr 21, 2022
i8ln should only be for using facing strings. This updates the code as
best as I could tell the difference, and also documents that expectation
for plugin writers.

closes pulp#2477
bmbouter added a commit to bmbouter/pulpcore that referenced this issue Apr 28, 2022
i8ln should only be for using facing strings. This updates the code as
best as I could tell the difference, and also documents that expectation
for plugin writers.

closes pulp#2477
bmbouter added a commit to bmbouter/pulpcore that referenced this issue Apr 28, 2022
i8ln should only be for using facing strings. This updates the code as
best as I could tell the difference, and also documents that expectation
for plugin writers.

closes pulp#2477
bmbouter added a commit to bmbouter/pulpcore that referenced this issue Apr 28, 2022
il8n should only be for using facing strings. This updates the code as
best as I could tell the difference, and also documents that expectation
for plugin writers.

closes pulp#2477
bmbouter added a commit to bmbouter/pulpcore that referenced this issue Apr 28, 2022
il8n should only be for using facing strings. This updates the code as
best as I could tell the difference, and also documents that expectation
for plugin writers.

closes pulp#2477
mdellweg pushed a commit that referenced this issue Apr 29, 2022
il8n should only be for using facing strings. This updates the code as
best as I could tell the difference, and also documents that expectation
for plugin writers.

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

Successfully merging a pull request may close this issue.

7 participants