SERVER-34864 FTDC should ignore irrelevant schema changes

9 files changed, 257 insertions, 32 deletions

diff --git a/src/mongo/db/ftdc/compressor.cpp b/src/mongo/db/ftdc/compressor.cpp
index 984698f7daf..5be24ceb1f7 100644
--- a/src/mongo/db/ftdc/compressor.cpp
+++ b/src/mongo/db/ftdc/compressor.cpp
@@ -45,8 +45,12 @@ using std::swap;
 StatusWith<boost::optional<std::tuple<ConstDataRange, FTDCCompressor::CompressorState, Date_t>>>
 FTDCCompressor::addSample(const BSONObj& sample, Date_t date) {
     if (_referenceDoc.isEmpty()) {
-        FTDCBSONUtil::extractMetricsFromDocument(sample, sample, &_metrics)
-            .status_with_transitional_ignore();
+        auto swMatchesReference =
+            FTDCBSONUtil::extractMetricsFromDocument(sample, sample, &_metrics);
+        if (!swMatchesReference.isOK()) {
+            return swMatchesReference.getStatus();
+        }
+
         _reset(sample, date);
         return {boost::none};
     }
diff --git a/src/mongo/db/ftdc/compressor_test.cpp b/src/mongo/db/ftdc/compressor_test.cpp
index 38ae3958a7b..db986687953 100644
--- a/src/mongo/db/ftdc/compressor_test.cpp
+++ b/src/mongo/db/ftdc/compressor_test.cpp
@@ -48,17 +48,17 @@ namespace mongo {
     ASSERT_TRUE(st.isOK());  \
     ASSERT_FALSE(st.getValue().is_initialized());
 
-#define ASSERT_SCHEMA_CHANGED(st)                                 \
-    ASSERT_TRUE(st.isOK());                                       \
-    ASSERT_TRUE(std::get<1>(st.getValue().get()) ==               \
-                FTDCCompressor::CompressorState::kSchemaChanged); \
-    ASSERT_TRUE(st.getValue().is_initialized());
-
-#define ASSERT_FULL(st)                                            \
-    ASSERT_TRUE(st.isOK());                                        \
-    ASSERT_TRUE(std::get<1>(st.getValue().get()) ==                \
-                FTDCCompressor::CompressorState::kCompressorFull); \
-    ASSERT_TRUE(st.getValue().is_initialized());
+#define ASSERT_SCHEMA_CHANGED(st)                   \
+    ASSERT_TRUE(st.isOK());                         \
+    ASSERT_TRUE(st.getValue().is_initialized());    \
+    ASSERT_TRUE(std::get<1>(st.getValue().get()) == \
+                FTDCCompressor::CompressorState::kSchemaChanged);
+
+#define ASSERT_FULL(st)                             \
+    ASSERT_TRUE(st.isOK());                         \
+    ASSERT_TRUE(st.getValue().is_initialized());    \
+    ASSERT_TRUE(std::get<1>(st.getValue().get()) == \
+                FTDCCompressor::CompressorState::kCompressorFull);
 
 class FTDCCompressorTest : public FTDCTest {};
 
@@ -127,7 +127,8 @@ TEST_F(FTDCCompressorTest, TestStrings) {
  */
 class TestTie {
 public:
-    TestTie() : _compressor(&_config) {}
+    TestTie(FTDCValidationMode mode = FTDCValidationMode::kStrict)
+        : _compressor(&_config), _mode(mode) {}
 
     ~TestTie() {
         validate(boost::none);
@@ -171,7 +172,7 @@ public:
             list = sw.getValue();
         }
 
-        ValidateDocumentList(list, _docs);
+        ValidateDocumentList(list, _docs, _mode);
     }
 
 private:
@@ -179,6 +180,7 @@ private:
     FTDCConfig _config;
     FTDCCompressor _compressor;
     FTDCDecompressor _decompressor;
+    FTDCValidationMode _mode;
 };
 
 // Test various schema changes
@@ -342,6 +344,114 @@ TEST_F(FTDCCompressorTest, TestSchemaChanges) {
     ASSERT_SCHEMA_CHANGED(st);
 }
 
+// Test various schema changes with strings
+TEST_F(FTDCCompressorTest, TestStringSchemaChanges) {
+    TestTie c(FTDCValidationMode::kWeak);
+
+    auto st = c.addSample(BSON("str1"
+                               << "joe"
+                               << "int1"
+                               << 42));
+    ASSERT_HAS_SPACE(st);
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int1"
+                          << 45));
+    ASSERT_HAS_SPACE(st);
+
+    // Add string field
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "str2"
+                          << "smith"
+                          << "int1"
+                          << 47));
+    ASSERT_HAS_SPACE(st);
+
+    // Reset schema by renaming a int field
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "str2"
+                          << "smith"
+                          << "int2"
+                          << 48));
+    ASSERT_SCHEMA_CHANGED(st);
+
+    // Remove string field
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int2"
+                          << 49));
+    ASSERT_HAS_SPACE(st);
+
+
+    // Add string field as last element
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int2"
+                          << 50
+                          << "str3"
+                          << "bar"));
+    ASSERT_HAS_SPACE(st);
+
+    // Reset schema by renaming a int field
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int1"
+                          << 51
+                          << "str3"
+                          << "bar"));
+    ASSERT_SCHEMA_CHANGED(st);
+
+    // Remove string field as last element
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int1"
+                          << 52));
+    ASSERT_HAS_SPACE(st);
+
+
+    // Add 2 string fields
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "str2"
+                          << "smith"
+                          << "str3"
+                          << "foo"
+                          << "int1"
+                          << 53));
+    ASSERT_HAS_SPACE(st);
+
+    // Reset schema by renaming a int field
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "str2"
+                          << "smith"
+                          << "str3"
+                          << "foo"
+                          << "int2"
+                          << 54));
+    ASSERT_SCHEMA_CHANGED(st);
+
+    // Remove 2 string fields
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int2"
+                          << 55));
+    ASSERT_HAS_SPACE(st);
+
+    // Change string to number
+    st = c.addSample(BSON("str1" << 12 << "int1" << 56));
+    ASSERT_SCHEMA_CHANGED(st);
+
+    // Change number to string
+    st = c.addSample(BSON("str1"
+                          << "joe"
+                          << "int1"
+                          << 67));
+    ASSERT_SCHEMA_CHANGED(st);
+}
+
 // Ensure changing between the various number formats is considered compatible
 TEST_F(FTDCCompressorTest, TestNumbersCompat) {
     TestTie c;
diff --git a/src/mongo/db/ftdc/controller_test.cpp b/src/mongo/db/ftdc/controller_test.cpp
index de88054aec9..f0317775bd1 100644
--- a/src/mongo/db/ftdc/controller_test.cpp
+++ b/src/mongo/db/ftdc/controller_test.cpp
@@ -204,7 +204,7 @@ TEST_F(FTDCControllerTest, TestFull) {
 
     auto alog = files[0];
 
-    ValidateDocumentList(alog, allDocs);
+    ValidateDocumentList(alog, allDocs, FTDCValidationMode::kStrict);
 }
 
 // Test we can start and stop the controller in quick succession, make sure it succeeds without
@@ -276,7 +276,7 @@ TEST_F(FTDCControllerTest, TestStartAsDisabled) {
 
     auto alog = files[0];
 
-    ValidateDocumentList(alog, allDocs);
+    ValidateDocumentList(alog, allDocs, FTDCValidationMode::kStrict);
 }
 
 }  // namespace mongo
diff --git a/src/mongo/db/ftdc/file_manager_test.cpp b/src/mongo/db/ftdc/file_manager_test.cpp
index 0a0b0367221..4605f308cd7 100644
--- a/src/mongo/db/ftdc/file_manager_test.cpp
+++ b/src/mongo/db/ftdc/file_manager_test.cpp
@@ -380,11 +380,11 @@ TEST_F(FTDCFileManagerTest, TestNormalCrashInterim) {
 
     // Validate old file
     std::vector<BSONObj> docs1 = {mdoc1, sdoc1, sdoc1};
-    ValidateDocumentList(files[0], docs1);
+    ValidateDocumentList(files[0], docs1, FTDCValidationMode::kStrict);
 
     // Validate new file
     std::vector<BSONObj> docs2 = {sdoc2, sdoc2, sdoc2, sdoc2};
-    ValidateDocumentList(files[1], docs2);
+    ValidateDocumentList(files[1], docs2, FTDCValidationMode::kStrict);
 }
 
 }  // namespace mongo
diff --git a/src/mongo/db/ftdc/file_writer_test.cpp b/src/mongo/db/ftdc/file_writer_test.cpp
index 81e3a741d24..e5a278427b7 100644
--- a/src/mongo/db/ftdc/file_writer_test.cpp
+++ b/src/mongo/db/ftdc/file_writer_test.cpp
@@ -198,7 +198,7 @@ private:
 
         _writer.close().transitional_ignore();
 
-        ValidateDocumentList(_path, _docs);
+        ValidateDocumentList(_path, _docs, FTDCValidationMode::kStrict);
     }
 
 private:
diff --git a/src/mongo/db/ftdc/ftdc_test.cpp b/src/mongo/db/ftdc/ftdc_test.cpp
index 11674ea2cad..631ca23397e 100644
--- a/src/mongo/db/ftdc/ftdc_test.cpp
+++ b/src/mongo/db/ftdc/ftdc_test.cpp
@@ -47,7 +47,23 @@
 
 namespace mongo {
 
-void ValidateDocumentList(const boost::filesystem::path& p, const std::vector<BSONObj>& docs) {
+namespace {
+
+BSONObj filteredFTDCCopy(const BSONObj& obj) {
+    BSONObjBuilder builder;
+    for (const auto& f : obj) {
+        if (FTDCBSONUtil::isFTDCType(f.type())) {
+            builder.append(f);
+        }
+    }
+    return builder.obj();
+}
+}  // namespace
+
+
+void ValidateDocumentList(const boost::filesystem::path& p,
+                          const std::vector<BSONObj>& docs,
+                          FTDCValidationMode mode) {
     FTDCFileReader reader;
 
     ASSERT_OK(reader.open(p));
@@ -61,20 +77,32 @@ void ValidateDocumentList(const boost::filesystem::path& p, const std::vector<BS
 
     ASSERT_OK(sw);
 
-    ValidateDocumentList(list, docs);
+    ValidateDocumentList(list, docs, mode);
 }
 
-void ValidateDocumentList(const std::vector<BSONObj>& docs1, const std::vector<BSONObj>& docs2) {
+void ValidateDocumentList(const std::vector<BSONObj>& docs1,
+                          const std::vector<BSONObj>& docs2,
+                          FTDCValidationMode mode) {
     ASSERT_EQUALS(docs1.size(), docs2.size());
 
     auto ai = docs1.begin();
     auto bi = docs2.begin();
 
     while (ai != docs1.end() && bi != docs2.end()) {
-        if (SimpleBSONObjComparator::kInstance.evaluate(*ai != *bi)) {
-            std::cout << *ai << " vs " << *bi << std::endl;
-            ASSERT_BSONOBJ_EQ(*ai, *bi);
+        if (mode == FTDCValidationMode::kStrict) {
+            if (SimpleBSONObjComparator::kInstance.evaluate(*ai != *bi)) {
+                std::cout << *ai << " vs " << *bi << std::endl;
+                ASSERT_BSONOBJ_EQ(*ai, *bi);
+            }
+        } else {
+            BSONObj left = filteredFTDCCopy(*ai);
+            BSONObj right = filteredFTDCCopy(*bi);
+            if (SimpleBSONObjComparator::kInstance.evaluate(left != right)) {
+                std::cout << left << " vs " << right << std::endl;
+                ASSERT_BSONOBJ_EQ(left, right);
+            }
         }
+
         ++ai;
         ++bi;
     }
diff --git a/src/mongo/db/ftdc/ftdc_test.h b/src/mongo/db/ftdc/ftdc_test.h
index fb9ada44f2f..c6f6be76e61 100644
--- a/src/mongo/db/ftdc/ftdc_test.h
+++ b/src/mongo/db/ftdc/ftdc_test.h
@@ -40,18 +40,38 @@ public:
 };
 
 /**
+ * Validation mode for tests, strict by default
+ */
+enum class FTDCValidationMode {
+    /**
+     * Compare BSONObjs exactly.
+     */
+    kStrict,
+
+    /**
+     * Compare BSONObjs by only comparing types FTDC compares about. FTDC ignores somes changes in
+     * the shapes of documents and therefore no longer reconstructs the shapes of documents exactly.
+     */
+    kWeak,
+};
+
+/**
  * Validate the documents in a file match the specified vector.
  *
  * Unit Test ASSERTs if there is mismatch.
  */
-void ValidateDocumentList(const boost::filesystem::path& p, const std::vector<BSONObj>& docs);
+void ValidateDocumentList(const boost::filesystem::path& p,
+                          const std::vector<BSONObj>& docs,
+                          FTDCValidationMode mode);
 
 /**
  * Validate that two lists of documents are equal.
  *
  * Unit Test ASSERTs if there is mismatch.
  */
-void ValidateDocumentList(const std::vector<BSONObj>& docs1, const std::vector<BSONObj>& docs2);
+void ValidateDocumentList(const std::vector<BSONObj>& docs1,
+                          const std::vector<BSONObj>& docs2,
+                          FTDCValidationMode mode);
 
 /**
  * Delete a file if it exists.
diff --git a/src/mongo/db/ftdc/util.cpp b/src/mongo/db/ftdc/util.cpp
index 243ba90b666..ecfc4db5f37 100644
--- a/src/mongo/db/ftdc/util.cpp
+++ b/src/mongo/db/ftdc/util.cpp
@@ -123,6 +123,47 @@ namespace FTDCBSONUtil {
 
 namespace {
 
+/**
+ * Iterate a BSONObj but only return fields that have types that FTDC cares about.
+ */
+class FTDCBSONObjIterator {
+public:
+    FTDCBSONObjIterator(const BSONObj& obj) : _iterator(obj) {
+        advance();
+    }
+
+    bool more() {
+        return !_current.eoo();
+    }
+
+    BSONElement next() {
+        auto ret = _current;
+        advance();
+        return ret;
+    }
+
+private:
+    /**
+     * Find the next element that is a valid FTDC type.
+     */
+    void advance() {
+        _current = BSONElement();
+
+        while (_iterator.more()) {
+
+            auto elem = _iterator.next();
+            if (isFTDCType(elem.type())) {
+                _current = elem;
+                break;
+            }
+        }
+    }
+
+private:
+    BSONObjIterator _iterator;
+    BSONElement _current;
+};
+
 StatusWith<bool> extractMetricsFromDocument(const BSONObj& referenceDoc,
                                             const BSONObj& currentDoc,
                                             std::vector<std::uint64_t>* metrics,
@@ -132,15 +173,14 @@ StatusWith<bool> extractMetricsFromDocument(const BSONObj& referenceDoc,
         return {ErrorCodes::BadValue, "Recursion limit reached."};
     }
 
-    BSONObjIterator itCurrent(currentDoc);
-    BSONObjIterator itReference(referenceDoc);
+    FTDCBSONObjIterator itCurrent(currentDoc);
+    FTDCBSONObjIterator itReference(referenceDoc);
 
     while (itCurrent.more()) {
         // Schema mismatch if current document is longer than reference document
         if (matches && !itReference.more()) {
             LOG(4) << "full-time diagnostic data capture schema change: currrent document is "
-                      "longer than "
-                      "reference document";
+                      "longer than reference document";
             matches = false;
         }
 
@@ -230,6 +270,24 @@ StatusWith<bool> extractMetricsFromDocument(const BSONObj& referenceDoc,
 
 }  // namespace
 
+bool isFTDCType(BSONType type) {
+    switch (type) {
+        case NumberDouble:
+        case NumberInt:
+        case NumberLong:
+        case NumberDecimal:
+        case Bool:
+        case Date:
+        case bsonTimestamp:
+        case Object:
+        case Array:
+            return true;
+
+        default:
+            return false;
+    }
+}
+
 StatusWith<bool> extractMetricsFromDocument(const BSONObj& referenceDoc,
                                             const BSONObj& currentDoc,
                                             std::vector<std::uint64_t>* metrics) {
diff --git a/src/mongo/db/ftdc/util.h b/src/mongo/db/ftdc/util.h
index 4816c534f96..04d453139ce 100644
--- a/src/mongo/db/ftdc/util.h
+++ b/src/mongo/db/ftdc/util.h
@@ -168,6 +168,11 @@ StatusWith<BSONObj> getBSONDocumentFromMetadataDoc(const BSONObj& obj);
  */
 StatusWith<std::vector<BSONObj>> getMetricsFromMetricDoc(const BSONObj& obj,
                                                          FTDCDecompressor* decompressor);
+
+/**
+ * Is this a type that FTDC find's interesting? I.e. is this a numeric or container type?
+ */
+bool isFTDCType(BSONType type);
 }  // namespace FTDCBSONUtil