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

Make GeneratedFileUsedEventArgs internal #9905

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Mar 21, 2024

Context

GeneratedFileUsedEventArgs do not need to be written to binlog. It's important to have the generated file being embedded into the binlog files, other than that the event doesn't bring additional value.

While forward compatibility of binlog allows us to introduce new events (or events additions) without breaking the viewer - there is still a minor impact that can confuse some users:

image

So it's safer to not to write the event now.

Mid-term, we should think about generalizing the event - so that it can be used for other scenarios (e.g. response file, .editorconfig file etc.), with the same structured format - #9906

Changes Made

Made GeneratedFileUsedEventArgs internal and dismounting sending it to the binlog (but kept the pushing of the generated file content itself - as that's the main gain brought by the feature).
Added Message representation of the event - for the verbouse logging via ConsoleLogger and FileLogger.

Testing

Kept existing tests.
Manual tests with binlog viewer and ConsoleLogger and FileLogger.

FYI @KirillOsenkov

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Should we then also adjust the way unrecognized events are presented in the UI so users are not confused?

@JanKrivanek
Copy link
Member Author

Should we then also adjust the way unrecognized events are presented in the UI so users are not confused?

This probably doesn't have correct answer :-)
Binlog viewer being more of a 'power-user tool' should not likely hide information about not being able to represent some data in the log.
Forward compatibility gives us more agility by allowing us to release changes without breaking the world - but it should be temporary or exceptional situations - where we plan to add support in viewer shortly (as otherwise why would we add a message to the binlog in the first place). This might chage though - if/once binlog will not be only diagnostic source but as well post-process-analysis source (but again - for a specific events where we consciously made the decision)

@ladipro
Copy link
Member

ladipro commented Mar 22, 2024

To be clear, I am not suggesting to not show the error/warning but instead of "reading errors encountered", it should probably say something like "you need a newer version to view all data in this log".

If we wanted to be fancy, new events could indicate how important they are, maybe simply encoded in the numerical value so the above could have a different wording/color according to the severity.

@JanKrivanek
Copy link
Member Author

I like the ideas!
We actually had some of those distinct messaging (the event is unknown, but there is a newer version available; the event is unknown while the versioning is not incremented; the event and version is unknown and new version of viewer doesn't exist):

KirillOsenkov/MSBuildStructuredLog@9481a34#diff-613f1ee280f18ac778935201a021faa4dc7cbd3ae8624e3d40ec020ab58daf8dR222

KirillOsenkov/MSBuildStructuredLog@9481a34#diff-613f1ee280f18ac778935201a021faa4dc7cbd3ae8624e3d40ec020ab58daf8dR263

KirillOsenkov/MSBuildStructuredLog@9481a34#diff-613f1ee280f18ac778935201a021faa4dc7cbd3ae8624e3d40ec020ab58daf8dR263

But then decided to simplify it. As - if the update is available - user will be notified anyways. If it is not available - then it's nonstandard situation that event is unknown.

@JanKrivanek JanKrivanek merged commit d225f2c into dotnet:main Mar 22, 2024
9 checks passed
@JanKrivanek JanKrivanek deleted the proto/new-event-args-to-internal branch March 22, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants