Added proxy logging #86
Conversation
| do { | ||
| // Skip any files; we only want directories | ||
| if ((findFileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0) | ||
| { | ||
| CProxyLogging::LogWarning(_T("dllmain::GetLatestVersionFolder - Skipping '%s', not a directory"), findFileData.cFileName); |
There was a problem hiding this comment.
LogWarning [](start = 31, length = 10)
Currently I'm logging a warning of each folder that fails parsing or pattern matching or missing profiler dll
Not sure if there are security or privacy implications #Resolved
There was a problem hiding this comment.
I'll remove these logwarnings since they're taking up a large amount of the eventlog for very little gain.
In reply to: 291384022 [](ancestors = 291384022)
|
|
||
| namespace MicrosoftInstrumentationEngine | ||
| { | ||
| class CEventLogger : |
There was a problem hiding this comment.
CEventLogger [](start = 10, length = 12)
This class is the proxy's version of CEventLoggerSink #Closed
|
Consider adding/changing existing tests to register using the proxy. #Closed |
|
I moved several classes from ProfilerProxy project to static ProfilerProxy.lib project in order to write my tests. In reply to: 499689819 [](ancestors = 499689819) |
WilliamXieMSFT
force-pushed
the
dev/willxie/proxy2
branch
from
4eb32bd
to
2951596
Compare
src/Common.Lib/Macros.h
Outdated
| @@ -29,6 +27,11 @@ namespace MicrosoftInstrumentationEngine | |||
| do { if (FAILED(hr = (EXPR))) { CLogging::LogError(_T("IfFailRet(") _T(#EXPR) _T(") failed in function ") __FUNCTIONT__); return hr; } } while (false) | |||
| #endif | |||
|
|
|||
| #ifndef IfFailRet_Proxy | |||
There was a problem hiding this comment.
IfFailRet_Proxy [](start = 8, length = 15)
Common.Lib should not be aware of proxy logging. You might want to define this inside the proxy project #Resolved
I added these SAL annotations to fix a build warning, I am confident there's no breaking API changes since the implementation in ProfilerManager.cpp (which had SAL annotations) had no changes needed. I'll verify with a build of Aerobee. In reply to: 500574167 [](ancestors = 500574167) Refers to: src/InstrumentationEngine.Api/InstrumentationEngine.h:924 in 2951596. [](commit_id = 2951596, deletion_comment = False) |
|
|
||
| HRESULT Initialize(); | ||
|
|
||
| void LogMessage(_In_ LPCWSTR wszMessage, _In_ va_list argptr); |
There was a problem hiding this comment.
Is the override keyword supported now, particularly for linux builds? it makes it clearer that you are implementing methods #Resolved
There was a problem hiding this comment.
Linux supports override, however these methods are not in the base class
In reply to: 292171420 [](ancestors = 292171420)
src/Common.Lib/EventLoggingBase.h
Outdated
|
|
||
| struct EventLogItem | ||
| { | ||
| EventLogItem(WORD wEventType, tstring tsEventLog) { |
There was a problem hiding this comment.
nit: use initializer #Resolved
src/Common.Lib/EventLoggingBase.h
Outdated
|
|
||
| class CEventLoggingBase | ||
| { | ||
| protected: |
There was a problem hiding this comment.
protected [](start = 0, length = 9)
Since we have protected functions to use these, can these members be private? #Resolved
|
|
||
| if (s_initialize.IsSuccessful()) | ||
| { | ||
| ++s_initCount; |
There was a problem hiding this comment.
s_initCount [](start = 10, length = 11)
is this really decremented each time it could be incremented? #Resolved
There was a problem hiding this comment.
Yes, the lifecycle of CProxyLogging is in dllmain's getclassobject which calls initialize and shutdown.
In reply to: 292172818 [](ancestors = 292172818)
There was a problem hiding this comment.
I'll add comments around this design for maintainability & aid in future changes
In reply to: 292173675 [](ancestors = 292173675,292172818)
There was a problem hiding this comment.
Discussed offline, there's a bug here if a thread calls initialize/shutdown before a second thread can call initialize (since the CInitOnce is not reset).
Fix is to move Shutdown() call on DllCanUnloadNow
In reply to: 292176770 [](ancestors = 292176770,292173675,292172818)
| private: | ||
| void AssertSucceeded(HRESULT hr) | ||
| { | ||
| Assert::IsTrue(SUCCEEDED(hr)); |
There was a problem hiding this comment.
IsTrue [](start = 20, length = 6)
Does this assert automatically pass along some useful callstack information to the test the result? #Resolved
There was a problem hiding this comment.
Both TestExplorer and running vstest.exe show stacktrace & assert message on failure
In reply to: 292173458 [](ancestors = 292173458)
… Address CR comments
| // [1]: SemanticVersion (eg. 1.0.0) | ||
| // [2]: Preview tag if exists (eg. -build12345) | ||
| // [3]: Debug tag if exists (eg. _Debug) | ||
| const std::wregex InstrumentationEngineVersion::versionRegex(_T("^(\\d+\\.\\d+\\.\\d+)(-build\\d+)?(_Debug)?$")); |
There was a problem hiding this comment.
\d+ [](start = 67, length = 4)
Just realized there's no checks for major version 1. #Resolved
docs/scenarios/azure.md
Outdated
|
|
||
| ## <a name="app-service">Azure App Service</a> | ||
|
|
||
| Microsoft Docs: [App Service Overview]() |
There was a problem hiding this comment.
() [](start = 38, length = 2)
No URL? #Resolved
There was a problem hiding this comment.
docs/scenarios/azure.md
Outdated
|
|
||
| Microsoft Docs: [App Service Overview]() | ||
|
|
||
| Azure App Service is a PaaS offering in Azure and provides hosting of web applications and sites as well as a Site Control Manager (SCM, also known as [Kudu](https://github.com/projectkudu/kudu) that provides administrative and management capabilities. |
There was a problem hiding this comment.
Azure App Se [](start = 0, length = 12)
Can paragraphs be split across lines so that we don't have to scroll during PRs? #Resolved
src/Common.Lib/EventLoggingBase.h
Outdated
| #include "tstring.h" | ||
| #include "CriticalSectionHolder.h" | ||
|
|
||
| struct EventLogItem |
There was a problem hiding this comment.
EventLogItem [](start = 7, length = 12)
Should there be a namespace for this stuff? #Resolved
…y from erroring out, nit fixes from comments
|
/azp run |
|
No pipelines are associated with this pull request. |
|
No pipelines are associated with this pull request. |
This change incorporates the reverted changes from #68
Changes related to Proxy Logging:
Other changes/fixes: