[2/2] feat: active configuration - report configuration to telemetry

[dmehala]

Description

This PR is second and final part of active configuration. This particular PR report configurations in app-started event and also report configuration updates received through Remote Configuration.

For reviewers

TODO

• Write unit tests.
• Validate with system-tests.

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-03-12 10:09:00

Comparing candidate commit 8861eb6 in PR branch dmehala/active-config-v2-part2 with baseline commit 04a350f in branch dmehala/active-config-v2.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

[dgoffredo]

Here is a first pass review for our meeting today.

My thoughts so far:

• Let's consider batching configuration updates, sending them only whenever the telemetry timer fires.
• Let's think more about how ConfigManager stores and modifies its state. I need to spend more time looking at it.
• Take a look at the possible deadlock that I commented on inline.
• How are we going to test this? I'll have to look more at our existing tests.

[dgoffredo]

Be sure to mention this in the comments at the top of tracer_telemetry.h. That is also a good time to audit the code for any telemetry event types that we forgot to mention before.

[dmehala]

addressed in e2bb8f4

[dgoffredo]

Here's an incomplete review, but I want to share these comments for our meeting today.

[dgoffredo]

Also, why setprecision(1)?

[dmehala]

To avoid:

[dgoffredo]

For our meeting: What is the meaning of "default" in this context?

[dgoffredo]

I just realized that I have two tabs open with two different code reviews. Flushing comments...

[dgoffredo]

Suggested change

	// A template class for managing dynamic configuration values.
	// A class template for managing dynamic configuration values.

[dgoffredo]

This is a bit of gate-keeping that I learned a while ago. It's not a class, it's a template.

[dmehala]

TIL

[dgoffredo]

Consider Value or Configurable instead of T. This is a matter of style, though.

[dgoffredo]

As you know, Optional<T>'s default value is nullopt, regardless of T.

[dgoffredo]

One way to centralize information about PropagationStyle is the following:

// in propagation_style.h
struct {
  StringView display_name;
  StringView telemetry_name;
} inline propagation_style_info[] = {
  {"Datadog", "datadog"},
  {"B3", "b3"}, // TODO: "b3multi"?
  {"W3C", "tracecontext"},
  {"none", "none"}
};

Then propagation_style_info[style].telemetry_name.

Really we could change the JSON encoder to use the telemetry names, so that there's only one representation.

Food for thought.

[dgoffredo]

Also, inline could be extern, but then you need a name for the size of the array 😠 (and possibly a name for the struct).

[dgoffredo]

I couldn't resist...

namespace {

template <typename Sequence, typename Func>
std::string join(const Sequence& elements, StringView separator, Func&& append_element) {
    auto iter = std::begin(elements);
    const auto end = std::end(elements);
    std::string result;
    if (iter == end) {
        return result;
    }
    append_element(result, *iter);
    for (++iter; iter != end; ++iter) {
        append(result, separator);
        append_element(result, *iter);
    }
    return result;
}

} // namespace

std::string join_tags(
    const std::unordered_map<std::string, std::string>& values) {
  return join(values, ",", [](std::string& result, const auto& entry) {
    const auto& [key, value] = entry;
    result += key;
    result += ':';
    result += value;
  });
}

std::string join(const std::vector<StringView>& values,
                 StringView separator) {
    return join(values, separator, [](std::string& result, StringView value) {
        append(result, value);
    });
}

std::string join_propagation_styles(
    const std::vector<PropagationStyle>& values) {
  return join(values, ",", [](std::string& result, PropagationStyle style) {
    switch (style) {
      case PropagationStyle::B3:
        result += "b3"; break;
      case PropagationStyle::DATADOG:
        result += "datadog"; break;
      case PropagationStyle::W3C:
        result += "tracecontext"; break;
      case PropagationStyle::NONE:
        result += "none"; break;
    }
  });
}

[dgoffredo]

I'd say "true" or "false", but the meaning is clear either way.

[dgoffredo]

Since you don't refer to the variable name in the contract, then it's safe to omit it, especially here where the interpretation of the parameter is obvious.

Alternatively, you could go full pedant and write it this way:

// Return a string representation of the specified boolean `value`.
// The result is "true" for `true` and "false" for `false`.
std::string to_string(bool value);

[dgoffredo]

Flushing comments...

edit: Oh, weird. Hope we didn't lose any.

[dgoffredo]

This will work, thanks for all of the thought put into it.

I'm wondering about alternative ways to factor ConfigManager's data members. Right now, you have one data member for remotely configurable thing, and share some code via class DynamicConfig. Then the work of managing the new values is in ConfigManager::update.

You set DD_TAGS by reforming the shared_ptr<const SpanDefaults>, because those tags live in SpanDefaults.

What happens when we need to separately configure a different property of SpanDefaults? Well, we'll almost certainly never need to. The other properties are more about span identity than about span annotation.

Still, I wonder if another protocol is warranted.

// TODO: Bad name, 0/10, do not recommend.
class DynamicallyConfigurable {
public:
  virtual std::vector<ConfigMetadata> update(const ConfigUpdate&) = 0;
  virtual void reset() { update({}); } // shrug
};

The benefit is that you can organize things by having constructors take dependencies, if any.

class ConfigManager {
  shared_ptr<const SpanDefaults> span_defaults_;
  shared_ptr<TraceSampler> trace_sampler_;
  SampleRateManager sample_rate_manager_;
  SamplingRulesManager sampling_rules_manager_;
  TagsManager tags_manager_;
  FooManager foo_manager_;
  ReportTracesManager report_traces_manager_;

  // ...
public:

  ConfigManager(...)
  : sample_rate_manager_(&trace_sampler_)
  , sampling_rules_manager_(&trace_sampler_)
  , tags_manager_(&span_defaults_)
  , foo_manager_(&span_defaults_)
  { ... }
};

vector<ConfigMetadata> ConfigManager::update(const 
vector<ConfigMetadata> ConfigManager::update(const ConfigUpdate& conf) {
  vector<ConfigMetadata> metadata;

  lock_guard lock(mutex_);
  append(metadata, sample_rate_manager_.update(conf));
  append(metadata, sampling_rules_manager_.update(conf));
  ...

The bookkeeping about the original value and such happens within each DynamicallyConfigurable derived class implementation.

Need to figure out how metadata gets updated when a reset (explicit or otherwise) changes the origin, so the "each manager returns a vector" isn't quite right.

I'm not confident that this is an improvement, but it's an idea.

Anyway, LGTM.