Skip to content
This repository has been archived by the owner on Apr 12, 2023. It is now read-only.

「ログ出力時に毎回ファイルを開いている」の修正 #67

Open
wants to merge 27 commits into
base: feature/log_improvement
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
de947fc
LogWriter 作成
Takym Mar 19, 2021
94b7d96
LoggerService のコンストラクタ変更に関する修正
Takym Mar 19, 2021
d0b4d44
CONTRIBUTORS.md 更新
Takym Mar 19, 2021
36b68b0
CreateLogRow を修正
Takym May 3, 2021
082a089
lock を Interlocked に変更
Takym May 3, 2021
016beb2
Merge branch 'master' into logging
Takym May 3, 2021
a9b4b62
Merge branch 'logging' of https://github.com/Takym/cocoa into logging
Takym May 3, 2021
e72f0da
Update Covid19Radar/Covid19Radar/Services/Logs/LogWriter.cs
Takym May 11, 2021
b459354
ソースファイルの書式を修正
Takym May 11, 2021
cfc16a0
Merge branch 'logging' of https://github.com/Takym/cocoa into logging
Takym May 11, 2021
a00563a
`CreateLogRow` を微修正
Takym May 11, 2021
03f7fab
フィールド名修正
Takym May 16, 2021
a68cac6
`_file` フィールドの書き換え処理を改善
Takym May 18, 2021
b4f15d8
`WriteLine(DateTime, string)` を改善
Takym May 18, 2021
2f6002d
`WriteLine(DateTime, string)` を微修正
Takym May 18, 2021
68b7d9f
`HEADER` の出力タイミングを修正
Takym May 18, 2021
f4986d2
テスト処理修正
Takym May 18, 2021
186a323
CreateLogRow、可読性&性能向上
Takym May 22, 2021
72a4f73
可読性&性能向上2
Takym May 22, 2021
c4bec33
`LogWriter.File` クラス改名
Takym Jun 5, 2021
09eaa47
LogWriter、フィールド変数の名前を変更
Takym Jun 5, 2021
c00b730
`LogWriter.LogFile` の仕組みを変更
Takym Jun 5, 2021
a4b32c0
`LogWriter.WriteLine` のローカル変数の名前を変更
Takym Jun 5, 2021
bc848ce
`LogWriter.LogFile.FileName` を **Path** に改名
Takym Jun 5, 2021
af59231
Merge branch 'cocoa-mhlw:develop' into logging
Takym Jun 11, 2021
b0a617c
Merge branch 'cocoa-mhlw:develop' into logging
Takym Jul 1, 2021
ac1f49f
Update unit tests for the logging system. (Takym/cocoa#4)
Takym Aug 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
- Tassana Thaveeteeratham (Thai Translation)
- Kotaro Sakamoto
- Koichi Yokota (Documentation)
- Takym (LogWriter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONTRIBUTORに関しては新しく項目を設けますね。


# Original Covid19Radar Beta Testers
- Nagahata Kenji
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public LogPeriodicDeleteReceiver()
{
var essensialService = new EssentialsService();
var logPathService = new LogPathService(new LogPathServiceAndroid());
loggerService = new LoggerService(logPathService, essensialService);
loggerService = new LoggerService(new LogWriter(logPathService, essensialService));
logFileService = new Covid19Radar.Services.Logs.LogFileService(loggerService, logPathService);
Comment on lines 62 to 65
Copy link
Collaborator

Choose a reason for hiding this comment

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

この部分だけServiceLocatorから外れていて、Singleton保てていないんですね。
話がややこしくなりそうなので、ここをServiceLocator化するPull Request出すことにします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LogPeriodicDeleteServiceAndroid の様に DependencyService.Resolve<T>() を使えば良いと思います。

Copy link
Contributor Author

@Takym Takym Jun 7, 2021

Choose a reason for hiding this comment

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

似た問題として #185#189 を確認しました。

}

Expand Down
3 changes: 2 additions & 1 deletion Covid19Radar/Covid19Radar/App.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public partial class App : PrismApplication
private ILoggerService LoggerService;
private ILogFileService LogFileService;

/*
/*
* The Xamarin Forms XAML Previewer in Visual Studio uses System.Activator.CreateInstance.
* This imposes a limitation in which the App class must have a default constructor.
* App(IPlatformInitializer initializer = null) cannot be handled by the Activator.
Expand Down Expand Up @@ -166,6 +166,7 @@ private static void RegisterCommonTypes(IContainer container)
container.Register<ILogPathService, LogPathService>(Reuse.Singleton);
container.Register<ILogPeriodicDeleteService, LogPeriodicDeleteService>(Reuse.Singleton);
container.Register<ILogUploadService, LogUploadService>(Reuse.Singleton);
container.Register<ILogWriter, LogWriter>(Reuse.Singleton);
container.Register<IEssentialsService, EssentialsService>(Reuse.Singleton);
container.Register<IUserDataService, UserDataService>(Reuse.Singleton);
container.Register<IExposureNotificationService, ExposureNotificationService>(Reuse.Singleton);
Expand Down
195 changes: 195 additions & 0 deletions Covid19Radar/Covid19Radar/Services/Logs/LogWriter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

#nullable enable

using System;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Threading;
using Covid19Radar.Common;

namespace Covid19Radar.Services.Logs
{
/// <summary>
/// ログの書き込みを行う機能を提供します。
/// </summary>
public interface ILogWriter
{
/// <summary>
/// ログを出力します。
/// </summary>
/// <param name="message">出力するメッセージです。</param>
/// <param name="method">実行中の関数の名前です。</param>
/// <param name="filePath">実行中の関数を格納しているソースファイルの名前です。</param>
/// <param name="lineNumber">実行中の関数のソースファイル内での行番号です。</param>
/// <param name="logLevel">ログレベルです。</param>
public void Write(string? message, string method, string filePath, int lineNumber, LogLevel logLevel);
}

/// <summary>
/// <see cref="Covid19Radar.Services.Logs.ILogWriter"/>の実装です。
/// </summary>
public sealed class LogWriter : ILogWriter
{
[ThreadStatic()]
private static StringBuilder? _sb;
Copy link
Collaborator

Choose a reason for hiding this comment

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

_sbという名前はローカル変数であればいいのですが、メンバ変数としては_logStringBuilderなどとした方がわかりやすいかと思います。

Copy link
Collaborator

Choose a reason for hiding this comment

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

後述しますがこのPRではLINQ周りは変えない方がいいと思うので_sbそのものが不要かもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_logStringBuilder ではなくキャッシュである事を強調する為に _sb_cache_sbCache などと命名するのはどうでしょうか?

private const string DATETIME_FORMAT = "yyyy/MM/dd HH:mm:ss.fffffff";
private static readonly string HEADER = CreateLogHeaderRow();
private readonly ILogPathService _log_path;
Copy link
Collaborator

Choose a reason for hiding this comment

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

名前がここだけがスネークケースなので、これはキャメルケースにした上でServiceをつけて_logPathServiceとする方が読みやすいと思います。

private readonly IEssentialsService _essentials;
Copy link
Collaborator

Choose a reason for hiding this comment

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

こちらも_essentialServiceとした方が、利用箇所からServiceとして登録されているものだとわかりやすいと思います。

private readonly Encoding _enc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここは_encodingの方が読みやすいと思います。
基本的にインスタンスメンバの名前は省略せず、べたっと書いた方が良いと思います。

private File? _file;

/// <summary>
/// 型'<see cref="Covid19Radar.Services.Logs.LogWriter"/>'の新しいインスタンスを生成します。
/// </summary>
/// <param name="logPath">ログファイルのパスを提供するサービスを指定します。</param>
/// <param name="essentials">環境情報を提供するサービスを指定します。</param>
public LogWriter(ILogPathService logPath, IEssentialsService essentials)
{
_log_path = logPath ?? throw new ArgumentNullException(nameof(logPath));
_essentials = essentials ?? throw new ArgumentNullException(nameof(essentials));
_enc = Encoding.UTF8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここはコンストラクタではなく、メンバを宣言するときに初期化で書いてしまった方が良い気がします。

}

/// <inheritdoc/>
public void Write(string? message, string method, string filePath, int lineNumber, LogLevel logLevel)
{
#if !DEBUG
if (logLevel == LogLevel.Verbose || logLevel == LogLevel.Debug) {
return;
}
#endif
try {
var jstNow = Utils.JstNow();
string row = CreateLogContentRow(message ?? string.Empty, method, filePath, lineNumber, logLevel, jstNow, _essentials);
Debug.WriteLine(row);
this.WriteLine(jstNow, row);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

この部分では日付を取得してから、WriteLine を呼び出しています。
しかし、WriteLine を実行するタイミングで日付が変更され、且つ _log_file が最新のファイルに更新された場合に、ファイルの開き直しが複数回行われる気がします。
日付の取得は WriteLine 内に移動した方が良さそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

下記の可能性が考えられます。

  1. 日付が変更される前に、スレッドAが WriteLine を呼び出します。
  2. 日付が変更された後に、スレッドBが WriteLine を呼び出します。
  3. スレッドB が _log_file を最新のファイルに更新します。
  4. スレッドA が _log_file を前日のファイルに更新します。
  5. 別のスレッドCがログを書き込むタイミングで、もう一度 _log_file を最新のファイルに更新します。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WriteLine の91行目の直後に日付を再取得すればこの問題は発生しませんが、この場合、前日のログを保持したログファイルが生成される可能性が生まれます。

} catch (Exception e) {
Debug.WriteLine(e.ToString());
}
}

private void WriteLine(DateTime jstNow, string line)
{
var file = _file;
string fname = _log_path.LogFilePath(jstNow);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここは省略せずfileNamefilePathの方が可読性が上がると思います。

if (file is null || file.FileName != fname) {
var newFile = new File(fname, _enc);
do {
if (Interlocked.CompareExchange(ref _file, newFile, file) == file) {
newFile.Writer.WriteLine(HEADER);
file?.Dispose();
file = newFile;
break;
}
Thread.Yield();
file = _file;
} while (file is null || file.FileName != fname);
}
file.Writer.WriteLine(line);
}

private static string CreateLogHeaderRow()
{
return CreateLogRow(
"output_date",
"log_level",
"message",
"method",
"file_path",
"line_number",
"platform",
"platform_version",
"model",
"device_type",
"app_version",
"build_number"
);
}

private static string CreateLogContentRow(
string message,
string method,
string filePath,
int lineNumber,
LogLevel logLevel,
DateTime jstDateTime,
IEssentialsService essentials)
{
return CreateLogRow(
jstDateTime.ToString(DATETIME_FORMAT),
logLevel.ToString(),
message,
method,
filePath,
lineNumber.ToString(),
essentials.Platform,
essentials.PlatformVersion,
essentials.Model,
essentials.DeviceType,
essentials.AppVersion,
essentials.BuildNumber
);
}

private static string CreateLogRow(params string[] cols)
{
_ = _sb is null ? _sb = new StringBuilder()
: _sb.Clear();

foreach (string col in cols) {
_sb.Append(",\"");
foreach (char ch in col) {
string? escaped = ch switch {
'\t' => "\\t", '\v' => "\\v",
'\r' => "\\r", '\n' => "\\n",
'\\' => "\\\\", '\"' => "\"\"",
_ => null
};
Comment on lines +167 to +172
Copy link
Collaborator

Choose a reason for hiding this comment

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

以前もお伝えしたとおり、CreateLogRow周りの変更について、現状(LINQ)から置き換えない方が良いと考えています。

本PRの主題は『「ログ出力時に毎回ファイルを開いている」の修正』なので、この変更を含めてしまうのは主題から外れるという認識です。性能面での改善は別PRにしてもらい、そこで検討するのが適切と考えます。

_ = escaped is null ? _sb.Append(ch)
: _sb.Append(escaped);
}
_sb.Append('\"');
}
_sb.Remove(0, 1);
return _sb.ToString();
}

private sealed class File : IDisposable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fileはとてもよく使われるクラス名なので、そこにかぶせずに「どのようなことに使うFileクラスなのか(通常のファイルクラスと何が違うのか)」がわかる名前にしてもらえるとうれしいです。

{
private readonly Encoding _enc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

省略せず_encodingでお願いします

private readonly Lazy<StreamWriter> _sw;
Copy link
Collaborator

Choose a reason for hiding this comment

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

省略せず_streamWriterなどわかりやすい名前にしてもらえるとありがたいです。


internal string FileName { get; }
internal StreamWriter Writer => _sw.Value;

internal File(string path, Encoding enc)
{
string dir = Path.GetDirectoryName(path);
if (!Directory.Exists(dir)) {
Directory.CreateDirectory(dir);
}

this.FileName = path;
_enc = enc;
_sw = new Lazy<StreamWriter>(this.OpenFile, LazyThreadSafetyMode.PublicationOnly);
}

private StreamWriter OpenFile()
{
var sw = new StreamWriter(new FileStream(this.FileName, FileMode.Append, FileAccess.Write, FileShare.ReadWrite), _enc);
sw.AutoFlush = true;
return sw;
}

public void Dispose()
{
this.Writer.Dispose();
}
}
}
}
Loading