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

add CER wrapper #12831

Merged
merged 12 commits into from
May 5, 2022
Merged

add CER wrapper #12831

merged 12 commits into from
May 5, 2022

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented Apr 26, 2022

This PR adds a CER wrapper in DynamoCoreWpf
It is windows only for now (has UI)
CER is not yet delivered as part of Dynamo. You can only use it if you have Revit, Civil or Robot installed (we can add more)
The CER windows will show up only if you have the CER feature flag enabled
I only added the minimum amount of information to the CER reports. There seem to be a lot more data that we can send to CER (we can probably enhance it as we go along)

Purpose

https://jira.autodesk.com/browse/DYN-4681

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

</upi_element>
</upi_element>
</UpiValueRetrieval>
</UpiDllConfig>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to find a way to generate this file at compile time (only the version information will change..). Could not figure out a way yet..(t4 templates are giving me a hard time)
As a last resort I can generate this file at runtime (when a crash is caught)

Copy link
Contributor

Choose a reason for hiding this comment

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

How is it generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is generated manually (following the CER guidelines)
It should correspond to every new release of dynamo (from the UPI configs)

@pinzart90 pinzart90 marked this pull request as ready for review April 28, 2022 14:56
private static extern uint GetCurrentThreadId();

[DllImport("dbghelp.dll")]
private static extern bool MiniDumpWriteDump(
Copy link
Member

Choose a reason for hiding this comment

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

is this from a reference somewhere? Maybe link would be good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dbghelp.dll is part of windows.
https://docs.microsoft.com/en-us/windows/win32/debug/dbghelp-versions
The docs do say that older versions might exist, but I think MiniDumpWriteDump is a very old API and it is probably on all versions of the dll.
It is also available as a redistributable if we want to have it published with Dynamo (but I think it will bring in other dependencies too)

var upiConfigFilePath = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "upiconfig.xml");

var cerArgs = $"/UPITOKEN {upiConfigFilePath} /DMP {miniDumpFilePath} /APPXML \"{appConfig}\" {extras}";
Process.Start(new ProcessStartInfo(CerToolLocation, cerArgs)).WaitForExit();
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason to WaitForExit()? Can't Dynamo just close and CER will go off and do its work?

Copy link
Contributor Author

@pinzart90 pinzart90 Apr 28, 2022

Choose a reason for hiding this comment

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

Yes it can. I wanted CER to behave the same as our inhouse crash report window (which is modal)

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 28, 2022

  1. Is it possible to add tests for the Dynamo side functionality?
  2. Do you think we need to update our analytics agreement in some way, we're sending full .dyns and preferences that may include local file paths? FYI @Amoursol
    2a. - is it clear how / when these reports will be generated for open source users, or only integrated ADSK versions of Dynamo?

@pinzart90
Copy link
Contributor Author

  1. Is it possible to add tests for the Dynamo side functionality?
  2. Do you think we need to update our analytics agreement in some way, we're sending full .dyns and preferences that may include local file paths? FYI @Amoursol
    2a. - is it clear how / when these reports will be generated for open source users, or only integrated ADSK versions of Dynamo?
  1. I am not sure if there is API available to check that a report was generated (and is valid). There is a way to override the waypoint to the backend server , but I will have to double check thi
    2 -There is a link in the CER UI - https://errorreport.autodesk.com/whatHappens.jsp?language=1033&version=6
    2.a - For now all benefit from CER (even open source users)

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 28, 2022

@pinzart90 the linked UI does not mention the upload including files/settings specifically, I guess it does mention the in memory program ...

</upi_element>
</upi_element>
</UpiValueRetrieval>
</UpiDllConfig>
Copy link
Contributor

Choose a reason for hiding this comment

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

How is it generated?

// Show the unhandled exception dialog so user can copy the
// crash details and report the crash if she chooses to.
viewModel.Model.OnRequestsCrashPrompt(null,
new CrashPromptArgs(e));
Copy link
Contributor

Choose a reason for hiding this comment

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

So now it's going to be the CER reporting dialog or our crash dialog in Dynamo - one or the other? Can it be both? In other words will the information we see in the standard Dynamo crash dialog be useful to show when the CER tool is enabled as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say one or the other.
CER collects everything that the old crash dialog was collecting, and more...

@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Apr 28, 2022

  1. Is it possible to add tests for the Dynamo side functionality?
  2. Do you think we need to update our analytics agreement in some way, we're sending full .dyns and preferences that may include local file paths? FYI @Amoursol
    2a. - is it clear how / when these reports will be generated for open source users, or only integrated ADSK versions of Dynamo?

I suppose we'll need to update PIA for Dynamo now that we are collecting a lot more information from users.

Comment on lines +18 to +21
public bool SendLogFile = true;
public bool SendSettingsFile = true;
public bool SendDynFile = true;
public bool SendRecordedCommands = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these flags going to be controlled somewhere (in the CER UI)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These flags are not exposed/controlled by the CER UI.
I added these flags expecting that I will expose the CrashReportTool as a public API for host integrators.
Some might want less to be collected ....

@Amoursol
Copy link
Contributor

  1. Is it possible to add tests for the Dynamo side functionality?
  2. Do you think we need to update our analytics agreement in some way, we're sending full .dyns and preferences that may include local file paths? FYI @Amoursol
    2a. - is it clear how / when these reports will be generated for open source users, or only integrated ADSK versions of Dynamo?

We will 100% need to runt his past legal, but so long as a user actively accepts something we are mostly likely legally covered. Is there a UI I can send on to James Connor @pinzart @mjkkirschner ?

@pinzart90
Copy link
Contributor Author

  1. Is it possible to add tests for the Dynamo side functionality?
  2. Do you think we need to update our analytics agreement in some way, we're sending full .dyns and preferences that may include local file paths? FYI @Amoursol
    2a. - is it clear how / when these reports will be generated for open source users, or only integrated ADSK versions of Dynamo?

We will 100% need to runt his past legal, but so long as a user actively accepts something we are mostly likely legally covered. Is there a UI I can send on to James Connor @pinzart @mjkkirschner ?

The .dyn, the settings files and the rest (which may be considered PIA) are only collected if the user clicks Send in the CER UI.
My hope is that the CER UI has enough information about what is being collected so that we would not have to change anything in Dynamo (legally). I think that is what Revit is doing too
Will contact legal to make sure
@aparajit-pratap FYI

/// <summary>
/// Struct mapping to MINIDUMP_EXCEPTION_INFORMATION for Win32 API
/// </summary>
[StructLayout(LayoutKind.Sequential, Pack = 4)]
Copy link
Member

Choose a reason for hiding this comment

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

maybe you answered this already, was this pack size documented somewhere or found by trial and error? it appears to make this struct smaller than if the default on an x64 system - I think there would be 4 bytes of padding between the ThreadId and ExceptionPointers if the default was used?
I guess this is to make the padding the same between architectures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason is this (from msdn docs):
https://docs.microsoft.com/en-us/cpp/dotnet/how-to-marshal-structures-using-pinvoke?view=msvc-170

there is no relationship between the native and managed version of the two structures other than their data layout. Therefore, it is vital that the managed version contains fields that are the same size and in the same order as the native version. (There is no mechanism for ensuring that the managed and native versions of the structure are equivalent, so incompatibilities will not become apparent until run time. It is the programmer's responsibility to ensure that the two structures have the same data layout.)

Because the members of managed structures are sometimes rearranged for performance purposes, it is necessary to use the [StructLayoutAttribute](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.structlayoutattribute) attribute to indicate that the structure are laid out sequentially. It is also a good idea to explicitly set the structure packing setting to be the same as that used by the native structure. (Although by default, Visual C++ uses an 8-byte structure packing for both managed code.)

I could not find info on how the native structure is packed...but I did find an example of c# pInvoking into the minidump function...and it specified packing with 4 bytes and it seems to work.
Example from here : https://www.pinvoke.net/default.aspx/dbghelp/MiniDumpWriteDump.html

Copy link
Contributor

@sm6srw sm6srw May 5, 2022

Choose a reason for hiding this comment

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

The default pack size for x64 is 16. This is normally not used by the Win32 apis if they want to keep binary backward compatibility for some reason. The way to figure out what to use is to look in the header files for Win32. So when I look at minidumpapiset.h I see in the beginning:

#include <pshpack4.h>

and in the end:

#include <poppack.h>

So 4 it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh..I searched for that header on my entire windows folder and program files.
Only now I realize that it was on the x86 program files...Damn
Thanks @sm6srw

@mjkkirschner
Copy link
Member

Hey @pinzart90 - a couple questions left -

  • No tests, even if we're just testing the pinvoke/struct signatures it seems useful to make sure they don't break. Not super important I guess.
  • Question about the struct packing / docs, if there was a reference for the structs that could be added to the code.
  • Tests are not running so maybe wait for a clean test run on master-5 before merging, or run it on master-15 self serve.

@pinzart90 pinzart90 requested a review from mjkkirschner May 5, 2022 02:55
@pinzart90 pinzart90 merged commit 318f2ab into master May 5, 2022
@pinzart90 pinzart90 deleted the cer_tool branch May 5, 2022 16:28
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.

6 participants