-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add ability to configure indexFormat #417
Conversation
I may have jumped the gun here. I may need to add WeekOfYear, WeekOfMonth and QuarterOfYear to the formatting options, but apparently C# string.Format() doesn't support that. /sigh Maybe q for quarter, w for Week of Month, and W for Week of Year? |
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.
Looks great! Thank you, @BuddhaBuddy1. Just one small suggestion
src/Microsoft.Diagnostics.EventFlow.Outputs.ElasticSearch/ElasticSearchOutput.cs
Outdated
Show resolved
Hide resolved
/azp run AT-Warsaw-rolling |
src/Microsoft.Diagnostics.EventFlow.Outputs.ElasticSearch/ElasticSearchOutput.cs
Show resolved
Hide resolved
src/Microsoft.Diagnostics.EventFlow.Outputs.ElasticSearch/ElasticSearchOutput.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.EventFlow.Outputs.ElasticSearch/ElasticSearchOutput.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.EventFlow.Outputs.ElasticSearch/ElasticSearchOutput.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.Diagnostics.EventFlow.Outputs.Tests/ElasticSearchOutputTests.cs
Show resolved
Hide resolved
Save Now() and pass around so there is no chance of it changing during processing. Fixed GetWeekOfMonth() bug, added test for it.
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.
Looks great, just one final suggestion. Thank you!
test/Microsoft.Diagnostics.EventFlow.Outputs.Tests/ElasticSearchOutputTests.cs
Show resolved
Hide resolved
/azp run AT-Warsaw-rolling |
Fix processing when multiple special format characters.
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.
Thank you @BuddhaBuddy1 ! Really appreciate you going through these iterations to make the change so nice.
src/Microsoft.Diagnostics.EventFlow.Outputs.ElasticSearch/ElasticSearchOutput.cs
Show resolved
Hide resolved
test/Microsoft.Diagnostics.EventFlow.Outputs.Tests/ElasticSearchOutputTests.cs
Show resolved
Hide resolved
@BuddhaBuddy1 you might have noticed that GH is giving us a warning sign for this PR, saying "branch is out of date with the base branch". This is because I have merged as small change to the build scripts while you were working on this PR. I can fix it for you when merging the PR, so don't worry too much, but you can also fix it on your side by taking the GH offer to "update" your branch (merging the latest changes). There should be no conflicts--the latest changes are not touching the files you have been working on. |
This is to implement issue 416.