-
Notifications
You must be signed in to change notification settings - Fork 11
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
Use periodic batching as base class #2
Use periodic batching as base class #2
Conversation
/// <summary> | ||
/// Container for all Microsoft Teams sink configuration. | ||
/// </summary> | ||
public class MicrosoftTeamsSinkOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to introduce a dedicated options container, make it responsible for the consistency of the passed values so that we don't need to validate them in multiple places. Something like as follows:
public class MicrosoftTeamsSinkOptions
{
private static readonly TimeSpan DefaultPeriod = ...;
private const int DefaultBatchSizeLimit = ...;
public MicrosoftTeamsSinkOptions(string webHookUri, IFormatProvider formatProvider = null, string title = null,
int? batchSizeLimit = null, TimeSpan? period = null)
{
if (webHookUri == null)
{
throw new ArgumentNullException(nameof(webHookUri));
}
if (string.IsNullOrEmpty(webHookUri))
{
throw new ArgumentException(nameof(webHookUri));
}
WebHookUri = webHookUri;
FormatProvider = formatProvider;
Title = title;
BatchSizeLimit = batchSizeLimit ?? DefaultBatchSizeLimit;
Period = period ?? DefaultPeriod;
}
public string WebHookUri { get; }
public IFormatProvider FormatProvider { get; }
public string Title { get; }
public int BatchSizeLimit { get; }
public TimeSpan Period { get; }
}
/// <summary> | ||
/// Optional: How many messages to send to microsoft teams at once. Defaults to 50. | ||
/// </summary> | ||
public int BatchSizeLimit { get; set; } = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are not doing any real batching yet - after 5 seconds we will be sending all messages one by one, it doesn't really make sense to have this setting anything but 1 by default. For this very reason, there is no point to wait 5 sec to send messages one by one. I suggest setting the default values as BatchSizeLimit=1
and Period=TimeSpan.FromSeconds(1)
to simulate simple buffering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true. We are sending messages one by one, but I believe it is still better to send them as a batch (one by one) or after x seconds rather than straight after each event is posted. I checked the microsoft documentation but I did not find a way to do it in any other way. But it is up to you what defaults you want ;-)
{ | ||
var message = CreateMessage(logevent); | ||
var json = JsonConvert.SerializeObject(message, jsonSerializerSettings); | ||
await Client.PostAsync(_options.WebHookUri, new StringContent(json)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't specify content type to StringContent
, it defaults to "text/plain". Even though it seems to be working with Microsoft Teams so far, it doesn't seem to be a correct content type to pass and might be not working in future. Better be on safe side and pass "application/json" as it was before
{ | ||
var message = CreateMessage(logevent); | ||
var json = JsonConvert.SerializeObject(message, jsonSerializerSettings); | ||
await Client.PostAsync(_options.WebHookUri, new StringContent(json)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add .ConfigureAwait(false)
private readonly string _webHookUri; | ||
private readonly IFormatProvider _formatProvider; | ||
private readonly string _title; | ||
private static HttpClient Client = new HttpClient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could be readonly
|
||
public MicrosoftTeamsSink(string webHookUri, IFormatProvider formatProvider, string title) | ||
private static readonly JsonSerializerSettings jsonSerializerSettings = new JsonSerializerSettings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: make it JsonSerializerSettings
from capital J
|
||
protected override async Task EmitBatchAsync(IEnumerable<LogEvent> events) | ||
{ | ||
foreach(var logevent in events) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: make it logEvent
or event
@@ -52,7 +56,7 @@ public static ILogger CreateLogger() | |||
|
|||
while (count-- > 0) | |||
{ | |||
using (var requestContext = await listener.AcceptAsync().WithTimeout(TimeSpan.FromSeconds(1)).ConfigureAwait(false)) | |||
using (var requestContext = await listener.AcceptAsync().WithTimeout(TimeSpan.FromSeconds(30)).ConfigureAwait(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
30 sec is too much I think, I would prefer if tests fail if messages are not sent for much shorter periods. I would say to set it to the actual period used in test logger + 1 sec to be safe.
/// </example> | ||
/// </summary> | ||
/// <param name="loggerSinkConfiguration">Instance of <see cref="LoggerSinkConfiguration"/> object.</param> | ||
/// <param name="webhookUrl">Microsoft teams post URI.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual parameter name is webHookUri
, not webhookUrl
/// <param name="title">Title that should be passed to the message.</param> | ||
/// <param name="formatProvider"><see cref="IFormatProvider"/> used to format the message.</param> | ||
/// <param name="restrictedToMinimumLevel"><see cref="LogEventLevel"/> value that specifies minimum logging level that will be allowed to be logged.</param> | ||
/// <returns>Instance of <see cref="LoggerConfiguration"/> object.</returns> | ||
public static LoggerConfiguration MicrosoftTeams( | ||
this LoggerSinkConfiguration loggerConfiguration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it loggerSinkConfiguration
to match with xml doc and other method overload
Thanks @Franklin89 ! |
@DixonDs Changed the base class and used the PeriodicBatching. Same way the slack sink works.