From 885c8d0ce4d399eda730be180762b6989cd01dae Mon Sep 17 00:00:00 2001 From: Max Golovanov Date: Wed, 16 Sep 2020 11:33:46 -0700 Subject: [PATCH] Code clean-up for OSS release --- examples/c/SampleC-Guest/demo.c | 5 +-- examples/c/SampleC/main.c | 5 +-- examples/cpp/SampleCpp/demo.c | 5 +-- examples/cpp/SampleCppMini/demo.c | 5 +-- lib/api/ContextFieldsProvider.hpp | 1 - lib/api/LogManagerFactory.cpp | 3 +- lib/api/LogManagerImpl.cpp | 8 ----- lib/api/Logger.cpp | 2 +- lib/api/capi.cpp | 3 +- lib/bond/CompactBinaryProtocolReader.hpp | 2 +- lib/config/RuntimeConfig_Default.hpp | 7 +--- lib/decoder/PayloadDecoder.cpp | 4 +-- lib/http/HttpRequestEncoder.cpp | 2 +- lib/http/HttpResponseDecoder.cpp | 2 +- lib/include/public/CAPIClient.hpp | 1 - lib/include/public/CommonFields.h | 1 - lib/offline/MemoryStorage.cpp | 4 --- lib/offline/OfflineStorageHandler.cpp | 14 +------- lib/offline/OfflineStorage_SQLite.cpp | 4 --- lib/offline/SQLiteWrapper.hpp | 5 +-- lib/offline/StorageObserver.cpp | 2 +- lib/pal/TaskDispatcher.hpp | 1 - lib/pal/WorkerThread.cpp | 5 +-- lib/pal/desktop/NetworkDetector.cpp | 2 +- .../WindowsDesktopNetworkInformationImpl.cpp | 2 +- lib/pal/desktop/WindowsEnvironmentInfo.hpp | 4 +-- lib/pal/posix/sysinfo_sources.cpp | 3 +- lib/pal/universal/WindowsRTHelpers.cpp | 2 +- lib/shared/LogManagerCX.cpp | 3 +- lib/shared/WindowsRTPlatformEventHandler.cpp | 1 - lib/stats/MetaStats.hpp | 1 - lib/system/EventProperty.cpp | 7 +--- lib/system/TelemetrySystem.cpp | 2 +- lib/system/TelemetrySystemBase.hpp | 2 -- lib/tpm/TransmissionPolicyManager.cpp | 4 --- lib/tpm/TransmissionPolicyManager.hpp | 2 +- tests/common/MockIRuntimeConfig.hpp | 2 -- tests/functests/BasicFuncTests.cpp | 35 ------------------ .../unittests/ContextFieldsProviderTests.cpp | 1 - tests/unittests/DataViewerCollectionTests.cpp | 20 ----------- tests/unittests/HttpClientManagerTests.cpp | 36 ------------------- tests/unittests/HttpClientTests.cpp | 1 - .../unittests/OfflineStorageTests_SQLite.cpp | 2 +- 43 files changed, 28 insertions(+), 195 deletions(-) diff --git a/examples/c/SampleC-Guest/demo.c b/examples/c/SampleC-Guest/demo.c index 864ffcfe..020cbab2 100644 --- a/examples/c/SampleC-Guest/demo.c +++ b/examples/c/SampleC-Guest/demo.c @@ -45,14 +45,13 @@ void test_c_api_guest() _STR("ver", "3.0"), // Represents the major and minor version of the extension _STR("time", "1979-08-12"), // Represents the event date time in Coordinated Universal Time(UTC) when the event was generated on the client.This should be in ISO 8601 format _INT("popSample", 100), // Represents the effective sample rate for this event at the time it was generated by a client - // FIXME: [MG] - iKey should only be needed if logging to alternate token, otherwise we should use the primaryToken by default _STR("iKey", API_KEY), // Represents an ID for applications or other logical groupings of events. _INT("flags", 0xffffffff), // Represents a collection of bits that describe how the event should be processed ... _STR("cV", "12345"), // Represents the Correlation Vector : A single field for tracking partial order of related telemetry events across component boundaries. // Customer Data fields go as part of userdata _STR("strKey", "value1"), _INT("intKey", 12345), - PII_STR("piiKey", "secret", 1), // TODO: copy-paste-translate the Pii Kind enum from C++ to C + PII_STR("piiKey", "secret", 1), // Part "X" demo - populating CS extension props // Common Data Extensions.App PII_STR("ext.app.userId", "jackfrost@microsoft.com", 1), @@ -61,8 +60,6 @@ void test_c_api_guest() evt_log(handle, event); evt_flush(handle); - // FIXME: [MG] - default settings are optimized for 'fast shutdown' and not giving enough time for SDK to upload the event logged. - // However, if you restart this sample - events from a previous run get uploaded. Modify the evt_upload(handle); evt_close(handle); } diff --git a/examples/c/SampleC/main.c b/examples/c/SampleC/main.c index 2e849c1b..7b3514e3 100644 --- a/examples/c/SampleC/main.c +++ b/examples/c/SampleC/main.c @@ -31,14 +31,13 @@ void test_c_api() _STR("ver", "3.0"), // Represents the major and minor version of the extension _STR("time", "1979-08-12"), // Represents the event date time in Coordinated Universal Time(UTC) when the event was generated on the client.This should be in ISO 8601 format _INT("popSample", 100), // Represents the effective sample rate for this event at the time it was generated by a client - // FIXME: [MG] - iKey should only be needed if logging to alternate token, otherwise we should use the primaryToken by default _STR("iKey", API_KEY), // Represents an ID for applications or other logical groupings of events. _INT("flags", 0xffffffff), // Represents a collection of bits that describe how the event should be processed ... _STR("cV", "12345"), // Represents the Correlation Vector : A single field for tracking partial order of related telemetry events across component boundaries. // Customer Data fields go as part of userdata _STR("strKey", "value1"), _INT("intKey", 12345), - PII_STR("piiKey", "secret", 1), // TODO: copy-paste-translate the Pii Kind enum from C++ to C + PII_STR("piiKey", "secret", 1), // Part "X" demo - populating CS extension props // Common Data Extensions.App PII_STR("ext.app.userId", "jackfrost@microsoft.com", 1), @@ -60,8 +59,6 @@ void test_c_api() evt_log(handle, event); evt_flush(handle); - // FIXME: [MG] - default settings are optimized for 'fast shutdown' and not giving enough time for SDK to upload the event logged. - // However, if you restart this sample - events from a previous run get uploaded. Modify the evt_upload(handle); evt_close(handle); } diff --git a/examples/cpp/SampleCpp/demo.c b/examples/cpp/SampleCpp/demo.c index bf38bc33..c1927d54 100644 --- a/examples/cpp/SampleCpp/demo.c +++ b/examples/cpp/SampleCpp/demo.c @@ -46,14 +46,13 @@ void test_c_api() _STR("ver", "3.0"), // Represents the major and minor version of the extension _STR("time", "1979-08-12"), // Represents the event date time in Coordinated Universal Time(UTC) when the event was generated on the client.This should be in ISO 8601 format _INT("popSample", 100), // Represents the effective sample rate for this event at the time it was generated by a client - // FIXME: [MG] - iKey should only be needed if logging to alternate token, otherwise we should use the primaryToken by default _STR(COMMONFIELDS_IKEY, API_KEY), // Represents an ID for applications or other logical groupings of events. _INT("flags", 0xffffffff), // Represents a collection of bits that describe how the event should be processed ... _STR("cV", "12345"), // Represents the Correlation Vector : A single field for tracking partial order of related telemetry events across component boundaries. // Customer Data fields go as part of userdata _STR("strKey", "value1"), _INT("intKey", 12345), - PII_STR("piiKey", "secret", 1), // TODO: copy-paste-translate the Pii Kind enum from C++ to C + PII_STR("piiKey", "secret", 1), // Part "X" demo - populating CS extension props // Common Data Extensions.App PII_STR("ext.app.userId", "jackfrost@microsoft.com", 1), @@ -62,8 +61,6 @@ void test_c_api() evt_log(handle, event); evt_flush(handle); - // FIXME: [MG] - default settings are optimized for 'fast shutdown' and not giving enough time for SDK to upload the event logged. - // However, if you restart this sample - events from a previous run get uploaded. Modify the evt_upload(handle); evt_close(handle); } diff --git a/examples/cpp/SampleCppMini/demo.c b/examples/cpp/SampleCppMini/demo.c index bad120b1..430cd71a 100644 --- a/examples/cpp/SampleCppMini/demo.c +++ b/examples/cpp/SampleCppMini/demo.c @@ -41,14 +41,13 @@ void test_c_api(const char * token) _STR("ver", "3.0"), // Represents the major and minor version of the extension _STR("time", "1979-08-12"), // Represents the event date time in Coordinated Universal Time(UTC) when the event was generated on the client.This should be in ISO 8601 format _INT("popSample", 100), // Represents the effective sample rate for this event at the time it was generated by a client - // FIXME: [MG] - iKey should only be needed if logging to alternate token, otherwise we should use the primaryToken by default _STR("iKey", token), // Represents an ID for applications or other logical groupings of events. _INT("flags", 0xffffffff), // Represents a collection of bits that describe how the event should be processed ... _STR("cV", "12345"), // Represents the Correlation Vector : A single field for tracking partial order of related telemetry events across component boundaries. // Customer Data fields go as part of userdata _STR("strKey", "value1"), _INT("intKey", 12345), - PII_STR("piiKey", "secret", 1), // TODO: copy-paste-translate the Pii Kind enum from C++ to C + PII_STR("piiKey", "secret", 1), // Part "X" demo - populating CS extension props // Common Data Extensions.App PII_STR("ext.app.userId", "jackfrost@microsoft.com", 1), @@ -57,8 +56,6 @@ void test_c_api(const char * token) evt_log(handle, event); evt_flush(handle); - // FIXME: [MG] - default settings are optimized for 'fast shutdown' and not giving enough time for SDK to upload the event logged. - // However, if you restart this sample - events from a previous run get uploaded. Modify the evt_upload(handle); evt_close(handle); diff --git a/lib/api/ContextFieldsProvider.hpp b/lib/api/ContextFieldsProvider.hpp index d824381b..c36817b3 100644 --- a/lib/api/ContextFieldsProvider.hpp +++ b/lib/api/ContextFieldsProvider.hpp @@ -14,7 +14,6 @@ namespace MAT_NS_BEGIN { - // TODO: [MG] - A/B EXP code has to be refactored into separate module class ContextFieldsProvider : public ISemanticContext { diff --git a/lib/api/LogManagerFactory.cpp b/lib/api/LogManagerFactory.cpp index fb9c6b41..6b64f5af 100644 --- a/lib/api/LogManagerFactory.cpp +++ b/lib/api/LogManagerFactory.cpp @@ -153,8 +153,7 @@ namespace MAT_NS_BEGIN shared[host].names.insert(name); } - // TODO: [MG] - if there was no module configuration supplied - // explicitly, then do we treat the client as host or guest? + // If there was no module configuration supplied explicitly, then do we treat the client as host or guest? c[CFG_BOOL_HOST_MODE] = (name == host); return shared[host].instance; } diff --git a/lib/api/LogManagerImpl.cpp b/lib/api/LogManagerImpl.cpp index 736131b0..22c0cc55 100644 --- a/lib/api/LogManagerImpl.cpp +++ b/lib/api/LogManagerImpl.cpp @@ -175,7 +175,6 @@ namespace MAT_NS_BEGIN cacheFilePath += filename; m_logConfiguration[CFG_STR_CACHE_FILE_PATH] = cacheFilePath; } - // TODO: [MG] - verify that cache file is writeable } if (m_logConfiguration.HasConfig(CFG_STR_TRANSMIT_PROFILES)) @@ -204,10 +203,6 @@ namespace MAT_NS_BEGIN { m_dataViewerCollection.RegisterViewer(m_dataViewer); } - else - { - // TODO: [MG] - register default data viewer implementation if enabled? - } if (m_taskDispatcher == nullptr) { @@ -417,7 +412,6 @@ namespace MAT_NS_BEGIN { GetSystem()->upload(); } - // FIXME: [MG] - make sure m_system->upload returns a status return STATUS_SUCCESS; } @@ -429,7 +423,6 @@ namespace MAT_NS_BEGIN { GetSystem()->pause(); } - // FIXME: [MG] - make sure m_system->pause returns a status return STATUS_SUCCESS; } @@ -441,7 +434,6 @@ namespace MAT_NS_BEGIN { GetSystem()->resume(); } - // FIXME: [MG] - make sure m_system->resume returns a status return STATUS_SUCCESS; } diff --git a/lib/api/Logger.cpp b/lib/api/Logger.cpp index 45fd80d2..aa44947f 100644 --- a/lib/api/Logger.cpp +++ b/lib/api/Logger.cpp @@ -190,7 +190,7 @@ namespace MAT_NS_BEGIN SetContext(k, EventProperty(v, pii)); }; - // TODO: [MG] - the goal of this method is to rewire the logger instance to any other ISemanticContext issued by SDK. + // The goal of this method is to rewire the logger instance to any other ISemanticContext issued by SDK. // SDK may provide a future option for a guest logger to opt-in into its own semantic context. The method will then // rewire from the default parent (Host LogManager context) to guest's sandbox context, i.e. enabling scenario where // several guests are attached to one host, but each guest has their own 'local' LogManager semantic context sandbox. diff --git a/lib/api/capi.cpp b/lib/api/capi.cpp index 38f1770e..a0f56ee7 100644 --- a/lib/api/capi.cpp +++ b/lib/api/capi.cpp @@ -277,7 +277,6 @@ evt_status_t mat_log(evt_context_t *ctx) } else { - // TODO: [MG] - verify guest configuration and decide if we need to overwrite logger->SetParentContext(nullptr); logger->LogEvent(props); ctx->result = EOK; @@ -395,7 +394,7 @@ extern "C" { break; case EVT_OP_VERSION: - // TODO: [MG] - add handling of ctx->data passed by caller inline stub. + // TODO: add handling of ctx->data passed by caller inline stub : // If there is API version mismatch between the stub and lib impl, then // depending on version passed down to SDK - lib may need to figure out // how to handle the mismatch. For now the onus of verifying for SDK diff --git a/lib/bond/CompactBinaryProtocolReader.hpp b/lib/bond/CompactBinaryProtocolReader.hpp index 4cd96209..6f939fa9 100644 --- a/lib/bond/CompactBinaryProtocolReader.hpp +++ b/lib/bond/CompactBinaryProtocolReader.hpp @@ -184,7 +184,7 @@ class CompactBinaryProtocolReader { bool ReadWString(std::string const& value) { - UNREFERENCED_PARAMETER(value); + UNREFERENCED_PARAMETER(value); uint32_t length; if (!ReadUInt32(length)) { return false; diff --git a/lib/config/RuntimeConfig_Default.hpp b/lib/config/RuntimeConfig_Default.hpp index 6a55dad4..e97ebdc2 100644 --- a/lib/config/RuntimeConfig_Default.hpp +++ b/lib/config/RuntimeConfig_Default.hpp @@ -66,8 +66,6 @@ namespace MAT_NS_BEGIN {"sample", {{"rate", 0}}}}; - // TODO: [MG] - add ability to re-Configure with new custom config on-demand - /// /// This class overlays a custom configuration provided by the customer /// on top of default configuration above (defaultRuntimeConfig) @@ -127,7 +125,6 @@ namespace MAT_NS_BEGIN virtual unsigned GetOfflineStorageResizeThresholdPct() override { - // FIXME: [MG] - add parameter for that return 99; } @@ -153,7 +150,6 @@ namespace MAT_NS_BEGIN virtual unsigned GetMinimumUploadBandwidthBps() override { - // FIXME: [MG] - add parameter for that return 0; } @@ -164,7 +160,6 @@ namespace MAT_NS_BEGIN virtual void SetEventLatency(std::string const& tenantId, std::string const& eventName, EventLatency latency) override { - // TODO: [MG] - currently we don't allow to override the event latency via ECS or runtime config tree UNREFERENCED_PARAMETER(tenantId); UNREFERENCED_PARAMETER(eventName); UNREFERENCED_PARAMETER(latency); @@ -187,7 +182,7 @@ namespace MAT_NS_BEGIN virtual Variant& operator[](const char* key) override { - return config[key]; // FIXME: [MG] - Error #116: LEAK 32 bytes + return config[key]; } virtual bool HasConfig(const char* key) override diff --git a/lib/decoder/PayloadDecoder.cpp b/lib/decoder/PayloadDecoder.cpp index a8265fb9..0b519795 100644 --- a/lib/decoder/PayloadDecoder.cpp +++ b/lib/decoder/PayloadDecoder.cpp @@ -494,8 +494,8 @@ namespace clienttelemetry { z_stream zs; memset(&zs, 0, sizeof(zs)); - // [MG]: must call inflateInit2 with -9 because otherwise - // it'd be searching for non-existing gzip header... + // Must call inflateInit2 with -9 because otherwise + // it'd be searching for non-existing gzip header. if (inflateInit2(&zs, -9) != Z_OK) { return false; diff --git a/lib/http/HttpRequestEncoder.cpp b/lib/http/HttpRequestEncoder.cpp index d31488b7..fc235be9 100644 --- a/lib/http/HttpRequestEncoder.cpp +++ b/lib/http/HttpRequestEncoder.cpp @@ -143,7 +143,7 @@ namespace MAT_NS_BEGIN { #if 0 - // XXX: [MG] - debug only + // Debug only: uncomment to set a breakpoint - decode-verify the payload before sending it. CsProtocol::Record result; bond_lite::CompactBinaryProtocolReader reader(ctx->body); bond_lite::Deserialize(reader, result); diff --git a/lib/http/HttpResponseDecoder.cpp b/lib/http/HttpResponseDecoder.cpp index 44406d76..71a4c17f 100644 --- a/lib/http/HttpResponseDecoder.cpp +++ b/lib/http/HttpResponseDecoder.cpp @@ -36,7 +36,7 @@ namespace MAT_NS_BEGIN { void HttpResponseDecoder::handleDecode(EventsUploadContextPtr const& ctx) { #ifndef NDEBUG - // XXX: [MG] - debug accessing object that's been already freed + // Debug only for Visual Studio: check if accessing object that's been already freed uint64_t ptr = (uint64_t)(ctx->httpResponse); assert(ptr != 0x00000000dddddddd); assert(ptr != 0xdddddddddddddddd); diff --git a/lib/include/public/CAPIClient.hpp b/lib/include/public/CAPIClient.hpp index 765dfceb..afe584d5 100644 --- a/lib/include/public/CAPIClient.hpp +++ b/lib/include/public/CAPIClient.hpp @@ -65,7 +65,6 @@ namespace MAT_NS_BEGIN return evt_configure(handle, config); } - // TODO: [MG] - header-only EventProperties class? evt_status_t log(evt_prop* evt) { return evt_log(handle, evt); diff --git a/lib/include/public/CommonFields.h b/lib/include/public/CommonFields.h index d4a6f6e2..c3fa01d5 100644 --- a/lib/include/public/CommonFields.h +++ b/lib/include/public/CommonFields.h @@ -122,7 +122,6 @@ #define DIAG_LEVEL_DEFAULT_MAX DIAG_LEVEL_OPTIONAL #endif -/* TODO: [MG] - this field does not exist in Common Schema. Move it away from DeviceInfo namespace */ #define SESSION_SDKUID "DeviceInfo.SDKUid" #define SETTER_METHOD(NAME) Set ## NAME diff --git a/lib/offline/MemoryStorage.cpp b/lib/offline/MemoryStorage.cpp index 7b3a81b7..4736dd2f 100644 --- a/lib/offline/MemoryStorage.cpp +++ b/lib/offline/MemoryStorage.cpp @@ -459,10 +459,6 @@ namespace MAT_NS_BEGIN { /// This method is not currently implemented bool MemoryStorage::ResizeDb() { - // TODO: [MG] - consider implementing reduction of in-ram queue at runtime. - // Scenario for this is if we already run with 16MB buffer, but would like - // to switch to 8MB on Control Plane config update - we'd have to flush - // the queue and never grow above the newly provisioned limit. LOG_WARN("Not implemented!"); return true; } diff --git a/lib/offline/OfflineStorageHandler.cpp b/lib/offline/OfflineStorageHandler.cpp index d96be041..cac4a0d2 100644 --- a/lib/offline/OfflineStorageHandler.cpp +++ b/lib/offline/OfflineStorageHandler.cpp @@ -38,7 +38,7 @@ namespace MAT_NS_BEGIN { m_queryDbSize(0), m_isStorageFullNotificationSend(false) { - // FIXME: [MG] - this code seems redundant / suspicious because OfflineStorage_SQLite.cpp is doing the same thing... + // TODO: [MG] - OfflineStorage_SQLite.cpp is performing similar checks uint32_t percentage = m_config[CFG_INT_RAMCACHE_FULL_PCT]; uint32_t cacheMemorySizeLimitInBytes = m_config[CFG_INT_RAM_QUEUE_SIZE]; if (percentage > 0 && percentage <= 100) @@ -228,18 +228,6 @@ namespace MAT_NS_BEGIN { { auto memDbSize = m_offlineStorageMemory->GetSize(); { -#if 0 - //check if Application needs to be notified - if ((memDbSize > m_memoryDbSizeNotificationLimit) && !m_isStorageFullNotificationSend) - { - // TODO: [MG] - do we really need in-memory DB size limit notifications here? - DebugEvent evt; - evt.type = DebugEventType::EVT_STORAGE_FULL; - evt.param1 = 1; - m_logManager.DispatchEvent(evt); - m_isStorageFullNotificationSend = true; - } -#endif // During flush, this will block on a mutex while records // are selected and removed from the cache (but will // not block for the subsequent handoff to persistent diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index 5992af07..bcde16bf 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -73,7 +73,6 @@ namespace MAT_NS_BEGIN { uint32_t ramSizeLimit = m_config[CFG_INT_RAM_QUEUE_SIZE]; m_DbSizeHeapLimit = ramSizeLimit; - // TODO: [MG] - this needs to be moved into constant const char* skipSqliteInit = m_config["skipSqliteInitAndShutdown"]; if (skipSqliteInit != nullptr) { @@ -139,7 +138,6 @@ namespace MAT_NS_BEGIN { // TODO: [MG] - this works, but may not play nicely with several LogManager instances // static SqliteStatement sql_insert(*m_db, m_stmtInsertEvent_id_tenant_prio_ts_data); - // TODO: [MG] - verify this codepath if (record.id.empty() || record.tenantToken.empty() || static_cast(record.latency) < 0 || record.timestamp <= 0) { LOG_ERROR("Failed to store event %s:%s: Invalid parameters", tenantTokenToId(record.tenantToken).c_str(), record.id.c_str()); @@ -260,7 +258,6 @@ namespace MAT_NS_BEGIN { #endif SqliteStatement releaseStmt(*m_db, m_stmtReleaseExpiredEvents); - // FIXME: [MG] - add error checking here if (!releaseStmt.execute(PAL::getUtcSystemTimeMs())) LOG_ERROR("Failed to release expired reserved events: Database error occurred"); else { @@ -685,7 +682,6 @@ namespace MAT_NS_BEGIN { } } - // FIXME: [MG] - migration code is missing for this scenario since we renamed property to latency!!! if (!SqliteStatement(*m_db, "CREATE TABLE IF NOT EXISTS " TABLE_NAME_EVENTS " (" "record_id" " TEXT," diff --git a/lib/offline/SQLiteWrapper.hpp b/lib/offline/SQLiteWrapper.hpp index d7e651f7..17c0bf11 100644 --- a/lib/offline/SQLiteWrapper.hpp +++ b/lib/offline/SQLiteWrapper.hpp @@ -346,7 +346,6 @@ namespace MAT_NS_BEGIN { // sqlite3 callback to translate result set into vector of vectors of string static int sqlite3_select_callback(void *p_data, int num_fields, char **p_fields, char **p_col_names) { - // TODO: [MG] - ideally we can return a map that uses column names as keys UNREFERENCED_PARAMETER(p_col_names); SQLRecords* records = static_cast(p_data); try { @@ -371,7 +370,6 @@ namespace MAT_NS_BEGIN { if (rc != SQLITE_OK) { LOG_DEBUG("rc=%u: %s", rc, (zErrMsg != nullptr) ? zErrMsg : sqlite3_errmsg(m_db)); if (zErrMsg) { - // TODO: [MG] - expose sqlite3_free via g_sqlite3Proxy ::sqlite3_free(zErrMsg); } } @@ -383,7 +381,6 @@ namespace MAT_NS_BEGIN { char *errmsg = nullptr; int result = 0; LOG_DEBUG("%s", sql); - // TODO: [MG] - expose sqlite3_exec via g_sqlite3Proxy result = ::sqlite3_exec(m_db, sql, callback, arg, &errmsg); if (!isOK(result, errmsg)) { @@ -393,7 +390,7 @@ namespace MAT_NS_BEGIN { } bool trylock() { - return isOK(sqlite3_exec("BEGIN EXCLUSIVE;")); // XXX: [MG] - ptr corruption in sqlite3DbMallocRawNN + return isOK(sqlite3_exec("BEGIN EXCLUSIVE;")); } /** diff --git a/lib/offline/StorageObserver.cpp b/lib/offline/StorageObserver.cpp index fa4e261d..66c438ed 100644 --- a/lib/offline/StorageObserver.cpp +++ b/lib/offline/StorageObserver.cpp @@ -46,7 +46,7 @@ namespace MAT_NS_BEGIN { return wantMore; }; - // TODO: [MG] - expose 120000 as a constant + // TODO: [MG] - expose 120000 as a configuration parameter if (!m_offlineStorage.GetAndReserveRecords(consumer, 120000, ctx->requestedMinLatency, ctx->requestedMaxCount)) { ctx->fromMemory = m_offlineStorage.IsLastReadFromMemory(); diff --git a/lib/pal/TaskDispatcher.hpp b/lib/pal/TaskDispatcher.hpp index 28d80539..91025b6f 100644 --- a/lib/pal/TaskDispatcher.hpp +++ b/lib/pal/TaskDispatcher.hpp @@ -21,7 +21,6 @@ namespace PAL_NS_BEGIN { namespace detail { - // TODO: [MG] - allow lambdas, std::function, functors, etc. template class TaskCall : public Task { diff --git a/lib/pal/WorkerThread.cpp b/lib/pal/WorkerThread.cpp index 435e6169..e7e6b2dc 100644 --- a/lib/pal/WorkerThread.cpp +++ b/lib/pal/WorkerThread.cpp @@ -24,7 +24,6 @@ namespace PAL_NS_BEGIN { protected: std::thread m_hThread; - // TODO: [MG] - investigate all the cases why we need recursive here std::recursive_mutex m_lock; std::timed_mutex m_execution_mutex; @@ -61,8 +60,7 @@ namespace PAL_NS_BEGIN { } catch (...) {}; - // TODO: [MG] - investigate how often that happens. - // Side-effect is that we have a queued work item discarded on shutdown. + // TODO: [MG] - investigate if we ever drop work items on shutdown. if (!m_queue.empty()) { LOG_WARN("m_queue is not empty!"); @@ -75,7 +73,6 @@ namespace PAL_NS_BEGIN { void Queue(MAT::Task* item) override { - // TODO: [MG] - show item type LOG_INFO("queue item=%p", &item); LOCKGUARD(m_lock); if (item->Type == MAT::Task::TimedCall) { diff --git a/lib/pal/desktop/NetworkDetector.cpp b/lib/pal/desktop/NetworkDetector.cpp index 08c55cab..e4fb84dc 100644 --- a/lib/pal/desktop/NetworkDetector.cpp +++ b/lib/pal/desktop/NetworkDetector.cpp @@ -94,7 +94,7 @@ namespace MAT_NS_BEGIN m_currentNetworkCost = _GetCurrentNetworkCost(); } //****************************************************************************************************************************** - // XXX: Bug in Visual Studio debug host: + // This code is required as a workaround for an issue in Visual Studio debug host mode: crash in W.N.C.dll // // onecoreuap\net\netprofiles\winrt\networkinformation\lib\handlemanager.cpp(132)\Windows.Networking.Connectivity.dll!0FBCFB9E: // (caller: 0FBCEE2C) ReturnHr(1) tid(4584) 80070426 The service has not been started. diff --git a/lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp b/lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp index 6ab16277..97ff6a9c 100644 --- a/lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp +++ b/lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp @@ -84,7 +84,7 @@ namespace PAL_NS_BEGIN { m_cost = NetworkCost_Unknown; #ifdef HAVE_MAT_NETDETECT if (m_isNetDetectEnabled) { - networkDetector = std::unique_ptr(new MATW::NetworkDetector()); // FIXME: [MG] - Error #99: POSSIBLE LEAK 352 direct bytes + 224 indirect bytes + networkDetector = std::unique_ptr(new MATW::NetworkDetector()); networkDetector->AddRef(); networkDetector->Start(); } diff --git a/lib/pal/desktop/WindowsEnvironmentInfo.hpp b/lib/pal/desktop/WindowsEnvironmentInfo.hpp index a909897f..687f6e72 100644 --- a/lib/pal/desktop/WindowsEnvironmentInfo.hpp +++ b/lib/pal/desktop/WindowsEnvironmentInfo.hpp @@ -42,8 +42,8 @@ namespace MAT_NS_BEGIN { } else { - // TODO: [MG] - fix this benign compiler warning - // Warning C6102 Using 'timeZone' from failed function call at line '46' + // TODO: [MG] - ref. https://docs.microsoft.com/en-us/windows/win32/api/timezoneapi/nf-timezoneapi-gettimezoneinformation + // Need to handle the case when API return TIME_ZONE_ID_UNKNOWN. Otherwise we may be reporting invalid timeZone.Bias return TimeZoneBiasToISO8601(timeZone.Bias + timeZone.StandardBias); } }; diff --git a/lib/pal/posix/sysinfo_sources.cpp b/lib/pal/posix/sysinfo_sources.cpp index 9d94e49e..e4f6bd2b 100644 --- a/lib/pal/posix/sysinfo_sources.cpp +++ b/lib/pal/posix/sysinfo_sources.cpp @@ -231,7 +231,6 @@ sysinfo_sources_impl::sysinfo_sources_impl() : sysinfo_sources() #endif #if defined(__APPLE__) - // FIXME: [MG] - This is not the most elegant way of obtaining it cache["devMake"] = "Apple"; cache["devModel"] = GetDeviceModel(); cache["osName"] = GetDeviceOsName(); @@ -275,7 +274,7 @@ sysinfo_sources_impl::sysinfo_sources_impl() : sysinfo_sources() #ifndef __APPLE__ add("appId", {"/proc/self/cmdline", "(.*)[ ]*.*[\n]*"}); #else - cache["appId"] = get_app_name(); // TODO: [MG] - verify this path + cache["appId"] = get_app_name(); #endif if (!get("devId").compare("")) diff --git a/lib/pal/universal/WindowsRTHelpers.cpp b/lib/pal/universal/WindowsRTHelpers.cpp index dd1e5ca7..d4c74746 100644 --- a/lib/pal/universal/WindowsRTHelpers.cpp +++ b/lib/pal/universal/WindowsRTHelpers.cpp @@ -7,7 +7,7 @@ namespace Microsoft { namespace Applications { - // TODO: [MG] - consider refactoring this to use a macro namespace + // TODO: [MG] - refactor this to use the macro namespace namespace Telemetry { namespace Windows { diff --git a/lib/shared/LogManagerCX.cpp b/lib/shared/LogManagerCX.cpp index 5d313af6..2ad884b9 100644 --- a/lib/shared/LogManagerCX.cpp +++ b/lib/shared/LogManagerCX.cpp @@ -175,8 +175,7 @@ namespace MATW_NS_BEGIN { if (!isInited) { transmitProfiles = json; - // [MG] - note that in this delay-loading scenario we cannot verify - // that the profile supplied by the customer is valid! + // Caller must ensure that the supplied JSON string is valid. return true; } diff --git a/lib/shared/WindowsRTPlatformEventHandler.cpp b/lib/shared/WindowsRTPlatformEventHandler.cpp index d53bb4c1..56f53955 100644 --- a/lib/shared/WindowsRTPlatformEventHandler.cpp +++ b/lib/shared/WindowsRTPlatformEventHandler.cpp @@ -1,5 +1,4 @@ #ifdef WIN10_CS -// TODO: [MG] - move this file to pal/winrt project #include "pch.h" #include "PlatformHelpers.h" #include "IPlatformEventReceiver.h" diff --git a/lib/stats/MetaStats.hpp b/lib/stats/MetaStats.hpp index df50f373..da2759f4 100644 --- a/lib/stats/MetaStats.hpp +++ b/lib/stats/MetaStats.hpp @@ -559,7 +559,6 @@ namespace MAT_NS_BEGIN { IRuntimeConfig& m_config; - // TODO: [MG] - allow stats configuration provisioning via IRuntimeConfig above StatsConfig m_statsConfig; /// diff --git a/lib/system/EventProperty.cpp b/lib/system/EventProperty.cpp index 71e5321a..917beb17 100644 --- a/lib/system/EventProperty.cpp +++ b/lib/system/EventProperty.cpp @@ -22,9 +22,6 @@ using namespace MAT; namespace MAT_NS_BEGIN { - // TODO: [MG] - time_ticks_t would benefit from an extra method: - // time_ticks_t::time_ticks_t(const std::time_t time) - /// /// Default constructor for an empty object /// @@ -252,7 +249,7 @@ namespace MAT_NS_BEGIN { case TYPE_STRING: { size_t len = strlen(source->as_string); - as_string = new char[len + 1]; // FIXME: [MG] - Error #14: LEAK 16 bytes + as_string = new char[len + 1]; memcpy((void*)as_string, (void*)source->as_string, len); as_string[len] = 0; break; @@ -314,7 +311,6 @@ namespace MAT_NS_BEGIN { EventProperty::EventProperty(const EventProperty& source) : type(source.type) { - // TODO: [MG] - memcpy is probably no longer needed here memcpy((void*)this, (void*)&source, sizeof(EventProperty)); copydata(&source); } @@ -326,7 +322,6 @@ namespace MAT_NS_BEGIN { EventProperty::EventProperty(EventProperty&& source) /* noexcept */ : type(source.type) { - // TODO: [MG] - memcpy is probably no longer needed here memcpy((void*)this, (void*)&source, sizeof(EventProperty)); copydata(&source); } diff --git a/lib/system/TelemetrySystem.cpp b/lib/system/TelemetrySystem.cpp index 1b16e405..f87ff015 100644 --- a/lib/system/TelemetrySystem.cpp +++ b/lib/system/TelemetrySystem.cpp @@ -45,7 +45,7 @@ namespace MAT_NS_BEGIN { // TODO: clarify how UTC subsystem initializes LogSessionData m_storageType=SessionStorageType::FileStore ? // We may not necessarily compile in SQLite support for UTC min-build. For now we assume nullptr. logSessionDataProvider.CreateLogSessionData(); - result&=stats.onStart(); // TODO: [MG]- readd this + result&=stats.onStart(); return result; }; diff --git a/lib/system/TelemetrySystemBase.hpp b/lib/system/TelemetrySystemBase.hpp index d581cb13..dbba7dfc 100644 --- a/lib/system/TelemetrySystemBase.hpp +++ b/lib/system/TelemetrySystemBase.hpp @@ -9,8 +9,6 @@ namespace MAT_NS_BEGIN { - // typedef std::function StateHandler; - typedef std::function EventHandler; /// diff --git a/lib/tpm/TransmissionPolicyManager.cpp b/lib/tpm/TransmissionPolicyManager.cpp index 38f1cb4b..990047b3 100644 --- a/lib/tpm/TransmissionPolicyManager.cpp +++ b/lib/tpm/TransmissionPolicyManager.cpp @@ -213,10 +213,6 @@ namespace MAT_NS_BEGIN { bool TransmissionPolicyManager::handleStart() { m_isPaused = false; - // TODO: [MG] - this implies that start would force the immediate upload, but - // some customers require to be able to start in a paused (no telemetry) state. - // We may avoid the issue if we schedule the first upload to happen 1 second - // after start scheduleUpload(std::chrono::seconds{1}, calculateNewPriority()); return true; } diff --git a/lib/tpm/TransmissionPolicyManager.hpp b/lib/tpm/TransmissionPolicyManager.hpp index 94c44be7..32ad7338 100644 --- a/lib/tpm/TransmissionPolicyManager.hpp +++ b/lib/tpm/TransmissionPolicyManager.hpp @@ -81,7 +81,7 @@ constexpr const char* const DefaultBackoffConfig = "E,3000,300000,2,1"; IBandwidthController* m_bandwidthController; std::recursive_mutex m_backoffMutex; - std::string m_backoffConfig { DefaultBackoffConfig }; // TODO: [MG] - move to config + std::string m_backoffConfig { DefaultBackoffConfig }; std::unique_ptr m_backoff; DeviceStateHandler m_deviceStateHandler; diff --git a/tests/common/MockIRuntimeConfig.hpp b/tests/common/MockIRuntimeConfig.hpp index 0a465c76..61ba0bc2 100644 --- a/tests/common/MockIRuntimeConfig.hpp +++ b/tests/common/MockIRuntimeConfig.hpp @@ -39,11 +39,9 @@ namespace testing { MOCK_METHOD0(GetTeardownTime, uint32_t()); MOCK_METHOD0(IsClockSkewEnabled, bool()); - // FIXME: [MG] - Google Mock doesn't support mocking operators virtual MAT::Variant & operator[](const char* key) { return config[key]; - // return (*this)[key]; }; }; diff --git a/tests/functests/BasicFuncTests.cpp b/tests/functests/BasicFuncTests.cpp index 0fc6a436..759b9733 100644 --- a/tests/functests/BasicFuncTests.cpp +++ b/tests/functests/BasicFuncTests.cpp @@ -1457,41 +1457,6 @@ TEST_F(BasicFuncTests, logManager_getLogManagerInstance_initializedReturnsNonnul LogManager::FlushAndTeardown(); } -#if 0 // XXX: [MG] - This test was never supposed to work! Because the URL is invalid, we won't get anything in receivedRequests - -TEST_F(BasicFuncTests, networkProblemsDoNotDropEvents) -{ - std::string badPortUrl; - badPortUrl.replace(0, badPortUrl.find('/', sizeof("http://")), "http://127.0.0.1:65535"); - - auto &configuration = LogManager::GetLogConfiguration(); - configuration[CFG_STR_COLLECTOR_URL] = badPortUrl.c_str(); - - { - Initialize(); - EventProperties event("event"); - event.SetProperty("property", "value"); - logger->LogEvent(event); - - // After initial delay of 2 seconds, the library will send a request, wait 3 seconds, send 1st retry, wait 3 seconds, send 2nd retry. - // Stop waiting 1 second before the 2nd retry (which will use the good URL again) and check that nothing has been received yet. - PAL::sleep(2000 + 2 * 3000 - 1000); - ASSERT_THAT(receivedRequests, SizeIs(0)); - - // If the code works correctly, the event was not dropped (despite being retried twice) - // because the retry was caused by network connectivity failures only - validate it. - waitForEvents(2, 2); - if (receivedRequests.size() > 1) - { - auto payload = decodeRequest(receivedRequests[receivedRequests.size() - 1], false); - } - - FlushAndTeardown(); - } - // EXPECT_THAT(payload.TokenToDataPackagesMap, Contains(Key("functests-tenant-token"))); -} -#endif - #if 0 // TODO: [MG] - re-enable this long-haul test TEST_F(BasicFuncTests, serverProblemsDropEventsAfterMaxRetryCount) { diff --git a/tests/unittests/ContextFieldsProviderTests.cpp b/tests/unittests/ContextFieldsProviderTests.cpp index e31587e0..b7edbfc3 100644 --- a/tests/unittests/ContextFieldsProviderTests.cpp +++ b/tests/unittests/ContextFieldsProviderTests.cpp @@ -6,7 +6,6 @@ using namespace testing; using namespace MAT; -// TODO: [MG] - this test would benefit from uncommenting a bunch of lines that have been commented by someone before.. TEST(ContextFieldsProviderTests, SetProperties) { ContextFieldsProvider ctx(nullptr); diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 1ea707fa..c827f162 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -105,36 +105,16 @@ TEST(DataViewerCollectionTests, RegisterViewer_DuplicateDataViewerRegistered_Thr TestDataViewerCollection dataViewerCollection { }; ASSERT_NO_THROW(dataViewerCollection.RegisterViewer(viewer)); - // TODO: [MG] - breaks on Mac - // UnitTests(50918,0x10751a5c0) malloc: can't allocate region - // *** mach_vm_map(size=18446744073709502464) failed (error code=3) - // UnitTests(50918,0x10751a5c0) malloc: *** set a breakpoint in malloc_error_break to debug - // /build/cpp_client_telemetry/tests/unittests/DataViewerCollectionTests.cpp:95: Failure - // Expected: dataViewerCollection.RegisterViewer(otherViewer) throws an exception of type std::invalid_argument. std::shared_ptr otherViewer = std::make_shared("sharedName", /*isTransmissionEnabled*/ false); CheckForExceptionOrAbort([&dataViewerCollection, &otherViewer]() { dataViewerCollection.RegisterViewer(otherViewer); }); } -// TODO: [MG] - this test is broken on Mac: -// UnitTests(50918,0x10751a5c0) malloc: can't allocate region -// *** mach_vm_map(size=18446744073709506560) failed (error code=3) -// UnitTests(50918,0x10751a5c0) malloc: *** set a breakpoint in malloc_error_break to debug -// /build/cpp_client_telemetry/tests/unittests/DataViewerCollectionTests.cpp:107: Failure -// Expected: dataViewerCollection.UnregisterViewer("NotRegisteredViewer") throws an exception of type std::invalid_argument. -// Actual: it throws a different type. TEST(DataViewerCollectionTests, UnregisterViewer_ViewerNameIsNullPtr_ThrowsInvalidArgumentException) { TestDataViewerCollection dataViewerCollection { }; CheckForExceptionOrAbort([&dataViewerCollection]() { dataViewerCollection.UnregisterViewer(nullptr); }); } -// TODO: [MG] - this test is broken on Mac: -// UnitTests(51202,0x1076055c0) malloc: can't allocate region -// *** mach_vm_map(size=18446744073709506560) failed (error code=3) -// UnitTests(51202,0x1076055c0) malloc: *** set a breakpoint in malloc_error_break to debug -// /build/cpp_client_telemetry/tests/unittests/DataViewerCollectionTests.cpp:125: Failure -// Expected: dataViewerCollection.UnregisterViewer("NotRegisteredViewer") throws an exception of type std::invalid_argument. -// Actual: it throws a different type. TEST(DataViewerCollectionTests, UnregisterViewer_ViewerNameIsNotRegistered_ThrowsInvalidArgumentException) { TestDataViewerCollection dataViewerCollection { }; diff --git a/tests/unittests/HttpClientManagerTests.cpp b/tests/unittests/HttpClientManagerTests.cpp index 60ff786b..389535ca 100644 --- a/tests/unittests/HttpClientManagerTests.cpp +++ b/tests/unittests/HttpClientManagerTests.cpp @@ -74,39 +74,3 @@ TEST_F(HttpClientManagerTests, HandlesRequestFlow) EXPECT_THAT(ctx->httpResponse, rspRef); EXPECT_THAT(ctx->durationMs, Gt(199)); } - -#if 0 -// TODO: [MG] - this test needs to be reworked because on Windows it makes sense -// to cancel all pending requests rather than one-by-one. -TEST_F(HttpClientManagerTests, CancelAbortsRequests) -{ - SimpleHttpRequest* req = new SimpleHttpRequest("HttpClientManagerTests"); - - auto ctx = new EventsUploadContext(); - ctx->httpRequestId = req->GetId(); - ctx->httpRequest = req; - ctx->recordIdsAndTenantIds["r1"] = "t1"; ctx->recordIdsAndTenantIds["r2"] = "t1"; - ctx->latency = EventLatency_Normal; - ctx->packageIds["tenant1-token"] = 0; - - IHttpResponseCallback* callback = nullptr; - EXPECT_CALL(httpClientMock, SendRequestAsync(ctx->httpRequest, _)) - .WillOnce(SaveArg<1>(&callback)); - hcm.sendRequest(ctx); - ASSERT_THAT(callback, NotNull()); - - EXPECT_CALL(httpClientMock, CancelRequestAsync(Eq("HttpClientManagerTests"))) - .WillOnce(Return()); - hcm.cancelAllRequests(); - - std::unique_ptr rsp(new SimpleHttpResponse("HttpClientManagerTests")); - rsp->m_result = HttpResult_Aborted; - - EXPECT_CALL(*this, resultRequestDone(ctx)) - .WillOnce(Return()); - IHttpResponse* rspRef = rsp.get(); - callback->OnHttpResponse(rsp.release()); - - EXPECT_THAT(ctx->httpResponse, rspRef); -} -#endif diff --git a/tests/unittests/HttpClientTests.cpp b/tests/unittests/HttpClientTests.cpp index 570fc97d..fefbb100 100644 --- a/tests/unittests/HttpClientTests.cpp +++ b/tests/unittests/HttpClientTests.cpp @@ -99,7 +99,6 @@ class HttpClientTests : public ::testing::Test, /** * This method temporarily copies SimpleHttpResponse to a responses buffer. - * TODO: [MG] - ideally we should create a copy-constructor for that. */ virtual SimpleHttpResponse* clone(IHttpResponse* inResponse) { diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index 6974890b..56db17e5 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -1,5 +1,5 @@ // Copyright (c) Microsoft. All rights reserved. -#if 0 // TODO: [MG] - re-enable the tests +#if 0 #include "common/Common.hpp" #include "common/MockIOfflineStorageObserver.hpp" #include "common/MockIRuntimeConfig.hpp"