-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add get AppContext properties diagnostic IPC command #86610
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #ifdef ENABLE_PERFTRACING | ||
| #include "ep-rt-coreclr.h" | ||
| #include <clrconfignocache.h> | ||
| #include <configuration.h> | ||
| #include <eventpipe/ds-process-protocol.h> | ||
| #include <eventpipe/ds-profiler-protocol.h> | ||
| #include <eventpipe/ds-dump-protocol.h> | ||
|
|
@@ -329,6 +330,45 @@ ds_rt_disable_perfmap (void) | |
| #endif // FEATURE_PERFMAP | ||
| } | ||
|
|
||
| static | ||
| uint32_t | ||
| ds_rt_appcontext_properties_get (dn_vector_ptr_t *props_array) | ||
| { | ||
| STATIC_CONTRACT_NOTHROW; | ||
| EP_ASSERT (props_array != NULL); | ||
|
|
||
| return Configuration::EnumerateKnobs([](const LPCWSTR& name, const LPCWSTR& value, void* context) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any properties that we might want to omit from here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the list for a very basic .NET 8 console app:
Other identifiers are added to this list when the app contains them in the Probably HOST_RUNTIME_CONTRACT can be skipped because its value is a in-memory address to the host runtime contract. Everything else seems to be directory and file paths of various types. My default position would be to only filter out HOST_RUNTIME_CONTRACT and leave everything else in in the list for reading values. In a future change that may allow modification of values, I can see some more of these being effectively made read-only, but I would have to defer to others on what's desirable to allowed to be read. @vitek-karas and @elinor-fung, do you have suggestions on what to filter (if any and where to do it) for this initial PR that enables reading the app config values? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The total size of all these switches can be in 10's kB. I assume that this event is going to be typically enabled by default. Do you really want to send 10's kB over the diagnostic channel during startup by default? I would ask a different question. What do you expect that this data is going to be used for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There's already an existing diagnostic command for collecting the entire environment block of the target process. I would hazard a guess that it's has similar size characteristics. We haven't seen any significant slowdown during startup nor have we received any customer feedback about it. The only one that I see right now that is significantly large is
Originally, we only wanted to get startup hook information, and append our own startup hook to the list which the process is held up during the diagnostics suspension (which occurs just before managed code execution is started); this request is #83756. It was suggested that this be generalized to get / set app context properties. Expanding it to this general concept also allows us to:
The following are hypothetical but likely candidates for use:
Those are the obvious ones that I can think of right now; I can probably dig out more scenarios where some of this data may be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. I'm dropping the "other" bonus usage and won't mention it anymore unless others keep poking at it. I just want startup hooks #83756 and runtime identifiers #74476 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can extend this contract via diagnostic command. The startup hook instantiation is load assembly + invocation of the well-known entrypoint. Load assembly has built-in synchronization that ensures that given assembly is loaded exactly once. And once the entrypoint is executing, it can do its own synchronization if there is a chance that it can be invoked multiple times by the diagnostic command. Once you have managed hook running in the process, you can do any inspection you want, with any precision you like. For example, you do not have to depend on guessing to figure out what kind of libc is loaded based on the RID. You can inspect the process from inside to see what kind of libc is in use. You do not need to depend on runtime adding built-in tracing for every piece of information that you need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Determining the libc type during diagnostic suspension is not for startup hook usage; it's for determining which variant of our profiler to use. For loading the profiler, either AddStartupProfiler and AttachProfiler diagnostic commands are invoked, both of which take a full file path to the profiler implementation. The former command is only usable during the suspension and does not evaluate whether the library is loadable or not because the actual load comes later after the runtime has been resumed. There is no feedback on whether the library is loaded or not, so we can't even just try one and fallback to the other; we need to know the RID before the runtime is resumed. For the latter, we could just try one and then the other if the first fails, but that feels like a waste if we already know what the RID is due to needing it for AddStartupProfiler.
I have never asked for that. There is no tracing involved. To make decisions on what needs to be loaded or enable at diagnostic suspension, we need diagnostic commands to get specific information (such as RID) because some features (such as profilers) require native-specific information and managed code is not executing at that time thus cannot help in a platform agnostic way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
RID is not appropriate for this need. For example, what are you going to do when you see RID If you need libc type to load the right profiler flavor, we should introduce diagnostic command that returns the libc type. Alternatively, you can find out the libc that the process has loaded using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I logged #74476 specifically for this a 9 months ago. I'll try to get to this if the diagnostics team cannot.
No, .NET Monitor is typically not running in the same process namespace in its primary scenario (multi-container environments such as Kubernetes, where .NET Monitor is typically running as a sidecar container and maybe be running as a different user). We can however use that as a fallback mechanism of almost last resort if it happens to be running in the same namespace, which we may do for runtime versions lower than .NET 8. |
||
| dn_vector_ptr_t * props_array = static_cast<dn_vector_ptr_t *>(context); | ||
|
|
||
| const LPCWSTR separator = W("="); | ||
| size_t name_len = u16_strlen(name); | ||
| size_t name_size = name_len * sizeof (ep_char16_t); // no null terminator | ||
| size_t separator_len = u16_strlen(separator); | ||
| size_t separator_size = separator_len * sizeof (ep_char16_t); // no null terminator | ||
| size_t value_len = u16_strlen(value); | ||
| size_t value_size = value_len * sizeof (ep_char16_t); // no null terminator | ||
|
|
||
| // <name>=<value>\0 | ||
| size_t str_size = name_size + separator_size + value_size + sizeof (ep_char16_t); // includes null terminator | ||
| ep_char16_t *str_entry = reinterpret_cast<ep_char16_t *>(malloc (str_size)); | ||
| if (!str_entry) | ||
| return E_OUTOFMEMORY; | ||
|
|
||
| ep_char16_t * dst = str_entry; | ||
| memcpy(dst, reinterpret_cast<const ep_char16_t *>(name), name_size); | ||
| dst += name_len; | ||
| memcpy(dst, reinterpret_cast<const ep_char16_t *>(separator), separator_size); | ||
| dst += separator_len; | ||
| memcpy(dst, reinterpret_cast<const ep_char16_t *>(value), value_size); | ||
| dst += value_len; | ||
| dst[0] = (ep_char16_t)'\0'; | ||
|
|
||
| dn_vector_ptr_push_back (props_array, str_entry); | ||
|
|
||
| return S_OK; | ||
| }, props_array); | ||
| } | ||
|
|
||
| /* | ||
| * DiagnosticServer. | ||
| */ | ||
|
|
||
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.
I've tapped into the
Configurationclass for this enumeration since it is the owner of the app config properties starting here and the diagnostic server/command handling is started later via here.The other statically available places to access this information were possibly from the host or coreclr exports and contracts, but I felt that it was not appropriate to add the enumeration API there for external consumption; also, based on discussion in #83756, there may be some desire to filter the properties before the
AppContextgets to report them, which feels like it would naturally fit in at configuration initialization. Finally, I will have a follow up PR that allows modification of these values, which the Configuration class can be more easily made to do than say the host and coreclr properties since those are already "sealed" at this point in time.