Update tag attribution

libs/config/README.md

@@ -3,10 +3,12 @@
 ## Overview
 
 The `Config` class serves as the configuration object for the Registry class, which manages how metrics are sent to 
-SpectatorD. The `Config` constructor takes two parameters. The first parameter is required and is a `WriterConfig` 
+SpectatorD. The `Config` constructor takes three parameters. The first parameter is required and is a `WriterConfig`
 object. This object defines how metrics will be sent to `SpectatorD`. The second parameter `extraTags` is an 
 unordered map of strings allowing you to provide additional tags to all of your metrics. Extra tags are additional 
-key-value pairs attached to all metrics and will be merged with environment-derived tags.
+key-value pairs attached to all metrics and will be merged with environment-derived tags; on a key collision the
+environment-derived value wins. The third parameter `includeEnvTags` defaults to `true` and controls whether the
+environment-derived tags are added (see [Environment Variables](#environment-variables)).
 
 ## Usage
 
@@ -32,10 +34,23 @@ Config config(WriterConfig(WriterTypes::Memory), tags);
 
 ## Environment Variables
 
-If the following environment variables are set and not empty, there key and value will also be read and applied to 
-your tags
-- `TITUS_CONTAINER_NAME`: Set the `nf.container` tag automatically
-- `TITUS_PROCESS_NAME`: Set the `nf.process` tag automatically
+If the following environment variables are set and not empty or whitespace-only, their values are added to your tags.
+They take precedence over the same keys passed in `extraTags`.
+
+- `NETFLIX_CONTAINER_NAME`, falling back to `TITUS_CONTAINER_NAME`: sets the `nf.container` tag.
+- `NETFLIX_PROCESS_NAME`, falling back to the deprecated `TITUS_PROCESS_NAME`: sets the `nf.process` tag. Set it
+  separately for each process, for example in the container's entrypoint: `NETFLIX_PROCESS_NAME=processA ./processA`.
+  A container that runs several processes must give each one its own name. Use a stable, low-cardinality name made of
+  the characters `-._A-Za-z0-9^~`; any other character is replaced with `_`.
+
+To leave out both tags, pass `false` as the third parameter, `includeEnvTags`. Do this in a process that reports
+metrics on behalf of other containers or processes, such as a relay or an agent like atlas-system-agent's pod monitor,
+so that its own container and process names do not overwrite the tags of the metrics it reports. Tags passed in
+`extraTags` are still added.
+
+```cpp
+Config config(WriterConfig(WriterTypes::UDP), {}, false);
+```
 
 ### Warning
 

libs/config/config.cpp

@@ -6,56 +6,66 @@
 
 #include <algorithm>
 #include <cctype>
+#include <cstdlib>
+#include <initializer_list>
 
 namespace spectator {
 
 struct ConfigConstants
 {
     static constexpr auto Container = "nf.container";
     static constexpr auto Process = "nf.process";
-    static constexpr auto EnvVarContainer = "TITUS_CONTAINER_NAME";
-    static constexpr auto EnvVarProcess = "TITUS_PROCESS_NAME";
+    static constexpr auto EnvVarContainer = "NETFLIX_CONTAINER_NAME";
+    static constexpr auto EnvVarContainerFallback = "TITUS_CONTAINER_NAME";
+    static constexpr auto EnvVarProcess = "NETFLIX_PROCESS_NAME";
+    // Deprecated: read only when NETFLIX_PROCESS_NAME is unset or blank. Set NETFLIX_PROCESS_NAME instead.
+    static constexpr auto EnvVarProcessFallback = "TITUS_PROCESS_NAME";
 };
 
-std::unordered_map<std::string, std::string> CalculateTags(
-    const std::unordered_map<std::string, std::string>& tags)
+// Sets tags[tag] from the first of env_vars that is set and not empty or whitespace-only. The value is stored as-is
+// (not trimmed).
+static void AddFirstNonEmpty(std::unordered_map<std::string, std::string>& tags, const char* tag,
+                             std::initializer_list<const char*> env_vars)
 {
-    std::unordered_map<std::string, std::string> valid_tags;
-
-    for (const auto& [fst, snd] : tags)
+    for (const char* env_var : env_vars)
     {
-        if (IsEmptyOrWhitespace(fst) == false && IsEmptyOrWhitespace(snd) == false)
+        const char* value = std::getenv(env_var);
+        if (value != nullptr && IsEmptyOrWhitespace(value) == false)
         {
-            valid_tags[fst] = snd;
+            tags[tag] = value;
+            return;
         }
     }
+}
 
-    const char* container_name = std::getenv(ConfigConstants::EnvVarContainer);
-    const char* process_name = std::getenv(ConfigConstants::EnvVarProcess);
+std::unordered_map<std::string, std::string> CalculateTags(
+    const std::unordered_map<std::string, std::string>& tags, bool includeEnvTags)
+{
+    std::unordered_map<std::string, std::string> valid_tags;
 
-    if (container_name != nullptr)
+    for (const auto& [fst, snd] : tags)
     {
-        std::string container_str(container_name);
-        if (IsEmptyOrWhitespace(container_str) == false)
+        if (IsEmptyOrWhitespace(fst) == false && IsEmptyOrWhitespace(snd) == false)
         {
-            valid_tags[ConfigConstants::Container] = container_str;
+            valid_tags[fst] = snd;
         }
     }
 
-    if (process_name != nullptr)
+    // Added after the passed-in tags, so the environment values win on a key collision.
+    if (includeEnvTags == true)
     {
-        std::string process_str(process_name);
-        if (IsEmptyOrWhitespace(process_str) == false)
-        {
-            valid_tags[ConfigConstants::Process] = process_str;
-        }
+        AddFirstNonEmpty(valid_tags, ConfigConstants::Container,
+                         {ConfigConstants::EnvVarContainer, ConfigConstants::EnvVarContainerFallback});
+        AddFirstNonEmpty(valid_tags, ConfigConstants::Process,
+                         {ConfigConstants::EnvVarProcess, ConfigConstants::EnvVarProcessFallback});
     }
 
     return valid_tags;
 }
 
-Config::Config(const WriterConfig& writerConfig, const std::unordered_map<std::string, std::string>& extraTags)
-    : m_extraTags(CalculateTags(extraTags)), m_writerConfig(writerConfig)
+Config::Config(const WriterConfig& writerConfig, const std::unordered_map<std::string, std::string>& extraTags,
+               bool includeEnvTags)
+    : m_extraTags(CalculateTags(extraTags, includeEnvTags)), m_writerConfig(writerConfig)
 {
     Logger::info("Config initialized with writer type: {}, buffer size: {}, location: {}",
                      WriterTypeToString(m_writerConfig.GetType()), m_writerConfig.GetBufferSize(), m_writerConfig.GetLocation());

libs/config/config.h

@@ -10,7 +10,12 @@ namespace spectator {
 class Config
 {
    public:
-    Config(const WriterConfig& writerConfig, const std::unordered_map<std::string, std::string>& extraTags = {});
+    // When includeEnvTags is true (the default), nf.container and nf.process are added from the environment and win
+    // over extraTags (see README.md). Pass false in a process that reports metrics on behalf of other containers or
+    // processes, such as a relay or an agent like atlas-system-agent's pod monitor, so its own identity does not
+    // overwrite their per-meter tags.
+    Config(const WriterConfig& writerConfig, const std::unordered_map<std::string, std::string>& extraTags = {},
+           bool includeEnvTags = true);
 
     ~Config() = default;
     Config(const Config& other) = default;

libs/config/test_config.cpp

@@ -43,14 +43,18 @@ class ConfigTest : public testing::Test
 {
    protected:
     // Create guards for each environment variable
+    EnvironmentVariableGuard netflixContainerGuard{"NETFLIX_CONTAINER_NAME"};
     EnvironmentVariableGuard containerGuard{"TITUS_CONTAINER_NAME"};
-    EnvironmentVariableGuard processGuard{"TITUS_PROCESS_NAME"};
+    EnvironmentVariableGuard processGuard{"NETFLIX_PROCESS_NAME"};
+    EnvironmentVariableGuard legacyProcessGuard{"TITUS_PROCESS_NAME"};
 
     void SetUp() override
     {
         // Ensure environment variables are unset before each test
+        netflixContainerGuard.unsetValue();
         containerGuard.unsetValue();
         processGuard.unsetValue();
+        legacyProcessGuard.unsetValue();
     }
 };
 
@@ -217,4 +221,159 @@ TEST_F(ConfigTest, MergingTags)
         EXPECT_EQ(config.GetExtraTags().at("custom"), "value");
         EXPECT_EQ(config.GetExtraTags().at("env"), "test");
     }
+}
+
+// Test the nf.container sources: NETFLIX_CONTAINER_NAME first, TITUS_CONTAINER_NAME as the fallback
+TEST_F(ConfigTest, ContainerNameSources)
+{
+    WriterConfig writerConfig(WriterTypes::Memory);
+
+    // NETFLIX_CONTAINER_NAME wins over TITUS_CONTAINER_NAME
+    {
+        netflixContainerGuard.setValue("app");
+        containerGuard.setValue("main");
+
+        Config config(writerConfig);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.container"), "app");
+    }
+
+    // An empty or whitespace-only NETFLIX_CONTAINER_NAME falls back to TITUS_CONTAINER_NAME
+    for (const char* blank : {"", "   "})
+    {
+        netflixContainerGuard.setValue(blank);
+        containerGuard.setValue("main");
+
+        Config config(writerConfig);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.container"), "main");
+    }
+
+    // NETFLIX_CONTAINER_NAME alone
+    {
+        netflixContainerGuard.setValue("app");
+        containerGuard.unsetValue();
+
+        Config config(writerConfig);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.container"), "app");
+    }
+
+    // Neither set: no nf.container
+    {
+        netflixContainerGuard.unsetValue();
+        containerGuard.unsetValue();
+
+        Config config(writerConfig);
+
+        EXPECT_FALSE(config.GetExtraTags().count("nf.container"));
+    }
+
+    // NETFLIX_CONTAINER_NAME wins over a passed-in nf.container
+    {
+        netflixContainerGuard.setValue("app");
+
+        std::unordered_map<std::string, std::string> tags = {{"nf.container", "passed-in"}};
+
+        Config config(writerConfig, tags);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.container"), "app");
+    }
+}
+
+// Test the nf.process sources: NETFLIX_PROCESS_NAME first, the deprecated TITUS_PROCESS_NAME as the fallback
+TEST_F(ConfigTest, ProcessNameSources)
+{
+    WriterConfig writerConfig(WriterTypes::Memory);
+
+    // NETFLIX_PROCESS_NAME wins over TITUS_PROCESS_NAME
+    {
+        processGuard.setValue("processA");
+        legacyProcessGuard.setValue("legacy-process");
+
+        Config config(writerConfig);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.process"), "processA");
+    }
+
+    // TITUS_PROCESS_NAME is used when NETFLIX_PROCESS_NAME is unset
+    {
+        processGuard.unsetValue();
+        legacyProcessGuard.setValue("legacy-process");
+
+        Config config(writerConfig);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.process"), "legacy-process");
+    }
+
+    // TITUS_PROCESS_NAME is used when NETFLIX_PROCESS_NAME is empty or whitespace-only
+    for (const char* blank : {"", "   "})
+    {
+        processGuard.setValue(blank);
+        legacyProcessGuard.setValue("legacy-process");
+
+        Config config(writerConfig);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.process"), "legacy-process");
+    }
+
+    // Empty values are ignored: no nf.process
+    {
+        processGuard.setValue("");
+        legacyProcessGuard.setValue("");
+
+        Config config(writerConfig);
+
+        EXPECT_FALSE(config.GetExtraTags().count("nf.process"));
+    }
+
+    // NETFLIX_PROCESS_NAME wins over a passed-in nf.process
+    {
+        processGuard.setValue("processA");
+        legacyProcessGuard.unsetValue();
+
+        std::unordered_map<std::string, std::string> tags = {{"nf.process", "passed-in"}};
+
+        Config config(writerConfig, tags);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 1);
+        EXPECT_EQ(config.GetExtraTags().at("nf.process"), "processA");
+    }
+}
+
+// Test that includeEnvTags = false leaves out the environment-derived tags and keeps the passed-in ones
+TEST_F(ConfigTest, IncludeEnvTagsFalse)
+{
+    WriterConfig writerConfig(WriterTypes::Memory);
+    netflixContainerGuard.setValue("app");
+    containerGuard.setValue("main");
+    processGuard.setValue("processA");
+    legacyProcessGuard.setValue("legacy-process");
+
+    // No environment-derived tags
+    {
+        Config config(writerConfig, {}, false);
+
+        EXPECT_TRUE(config.GetExtraTags().empty());
+    }
+
+    // Passed-in tags are kept, including nf.container and nf.process
+    {
+        std::unordered_map<std::string, std::string> tags = {
+            {"custom", "value"}, {"nf.container", "passed-in"}, {"nf.process", "passed-in"}};
+
+        Config config(writerConfig, tags, false);
+
+        EXPECT_EQ(config.GetExtraTags().size(), 3);
+        EXPECT_EQ(config.GetExtraTags().at("custom"), "value");
+        EXPECT_EQ(config.GetExtraTags().at("nf.container"), "passed-in");
+        EXPECT_EQ(config.GetExtraTags().at("nf.process"), "passed-in");
+    }
 }

spectator/test_registry.cpp

@@ -1,5 +1,6 @@
 #include <gtest/gtest.h>
 #include <cstdint>
+#include <cstdlib>
 
 #include <registry.h>
 #include <writer_test_helper.h>
@@ -8,6 +9,23 @@
 
 using namespace spectator;
 
+// Config adds nf.container and nf.process from these environment variables. Clear them before the tests run, so the
+// exact-output checks below also pass on hosts where the platform sets them. Registered from a global initializer
+// because this test uses gtest_main.
+class ClearEnvTagsEnvironment : public testing::Environment
+{
+   public:
+    void SetUp() override
+    {
+        unsetenv("NETFLIX_CONTAINER_NAME");
+        unsetenv("TITUS_CONTAINER_NAME");
+        unsetenv("NETFLIX_PROCESS_NAME");
+        unsetenv("TITUS_PROCESS_NAME");
+    }
+};
+
+testing::Environment* const clearEnvTagsEnvironment = testing::AddGlobalTestEnvironment(new ClearEnvTagsEnvironment);
+
 TEST(RegistryTest, Close)
 {
     auto config = Config(WriterConfig(WriterTypes::Memory));