Report teh project name that we were unable to recover a symbol from.#46237
Report teh project name that we were unable to recover a symbol from.#46237CyrusNajmabadi merged 5 commits intodotnet:masterfrom
Conversation
| { | ||
| throw new InvalidOperationException( | ||
| $"We should always be able to resolve a symbol back on the host side:\r\n{SymbolKeyData}\r\n{failureReason}"); | ||
| $"We should always be able to resolve a symbol back on the host side:\r\n{project.Name}\r\n{SymbolKeyData}\r\n{failureReason}"); |
There was a problem hiding this comment.
Do we need to worry about this from a PII standpoint? Does the exception message get sent or logged anywhere?
There was a problem hiding this comment.
this is already causing a watson crash dump. so we're already collecting all that information. i would presume normal watson pii takes effect here as this is all going through that system.
There was a problem hiding this comment.
Yeah, any full memory dump would already contain 'project', so the information is already in the heap. The only risk here would be triage dumps which would capture the exception but not the local variables, or if this gets logged into some other system.
|
@jinujoseph i will likely want to take this in 16.7 as a followup to #46043. Effectively it includes a little more targeted information gathering as @sharwell was able to hit things in practice and narrow it down to a specific type of symbol. This may hopefully give enough information to track down the root on this. |
|
waiting on @sharwell or @jasonmalinowski for review. |
| { | ||
| throw new InvalidOperationException( | ||
| $"We should always be able to resolve a symbol back on the host side:\r\n{SymbolKeyData}\r\n{failureReason}"); | ||
| $"We should always be able to resolve a symbol back on the host side:\r\n{project.Name}\r\n{SymbolKeyData}\r\n{failureReason}"); |
There was a problem hiding this comment.
Yeah, any full memory dump would already contain 'project', so the information is already in the heap. The only risk here would be triage dumps which would capture the exception but not the local variables, or if this gets logged into some other system.
|
|
||
| bool TryResolveLocation(Location loc, int index, out SymbolKeyResolution resolution, out string? reason) | ||
| { | ||
| var resolutionOpt = reader.ResolveLocation(loc); |
There was a problem hiding this comment.
Why the resolution/resolutionOpt? I only see a single use of 'resolution' and that's only to grab the symbol so worth simplifying it?
| var resolutionOpt = reader.ResolveLocation(loc); | ||
| if (resolutionOpt == null) | ||
| { | ||
| reason = $"location {index} failed to resolve"; |
There was a problem hiding this comment.
Do you want the string to also include 'location' rather than the index? It seems like that'd be more useful and also prevents the need of passing index?
| } | ||
| totalFailureReason = totalFailureReason == null | ||
| ? $"({reason})" | ||
| : $"({totalFailureReason} -> {reason})"; |
There was a problem hiding this comment.
This is going to add up to a string like (((a) -> b) -> c) with a bunch of nested parenthesis; expected?
There was a problem hiding this comment.
yup. that way you can make out the 'tree/nesting' structure. i.e. you might have a few failures, which themselves have nested failures. i.e.:
(location 1 failed because (foo failed bacause (bar failed ...))) (location 2 failed because (foo failed because (...))))
not havin thte parens makes it ambiguous how this ties together.
Backport #46237 to dev16.7: Report teh project name that we were unable to recover a symbol from.
No description provided.