-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 Extended messages data to binary log #9198
Conversation
@KirillOsenkov I suppose we shall 1st release forward compatible log viewer. We can do it in two phases:
While I can easily do 1 for 2 I might need some help. |
Do we have a design document or a wiki page outlining the problem space, the motivation and the proposed approach? What do we know about the existing usage of the "exotic" event args? Do we have a list of teams/scenarios depending on these? As we discover scenarios and knowledge, let's add to the doc. I'm sure we'll break someone, so would be nice to point them to the doc. I've always felt uncertainty about the exotic event args - they're ancient and it's unclear who if anyone is still using them and for what. Would also help to know these scenarios so we can test and validate. |
Research of usage custom event arguments was concluded in: Custom event arguments has been deprecated for dotnet core msbuild in favor of new messages containing general purpose transparent payload. See #8917 I would not care too much about binlog supporting rare needs for custom events args. We believe, that there can be substantial value in capturing and showing such info in log viewer. And this PR is the part of such effort. |
Sounds good. I'm missing a ton of context, but superficially everything seems fine. See if you can utilize BuildEventArgsFieldFlags in any way: Perhaps you can avoid writing a boolean to see if extended data is present if you can write a single bit in the flags. |
@KirillOsenkov I have totally missed |
It is ready for review. I just left it in Draft so it is not merged before we release compatible log viewer. |
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 good!
Looks OK as far as I can see |
Add Extended messages data to binary log
Fixes #9091
Context
We have introducing new extended event args (error, warn, message, custom) so time ago.
This PR is about logging extended data into binary log and will serve for modifying bin log viewer as well.
Changes Made
Add
BuildEventArgsFieldFlags.Extended
and implement related code.Testing
Unit tests
Notes