remote config: more code review comments

[dgoffredo]

These are my latest ideas and suggestions for #74.

The changes proposed #74 are looking very good. The design is sound, the code is straightforward, and the tests make sense.

In no particular order, here's what I'm thinking about when reviewing that code:

What is TracerID?

This has been a long series of conversations among me, Damien, Caleb, remote config maintainers, and others on Slack. I essentially throw up my hands trying to understand all of the potential relationships among:

• runtime ID
• client ID
• service, environment
• process, thread
• class Tracer

Instead, I propose we choose a specifically vague abstraction in lieu of TracerID. Here I call it "TracerSignature". It's a weasel word, "signature," what does it mean? Well, it's kind of an ID, but not really an ID. I was tempted by TracerInfo but that's just giving up. What keeps me away from "TracerID" is that I wonder identity in what sense?

TracerSignature, on the other hand, is documented as meaning only a bag of data that is used in conjunction with telemetry and with remote config. Nothing more.

This vagueness and verbosity lacks elegance, but I think that it's an improvement over an under-defined notion of an "ID" for a tracer.

I'm not 100% sold on "signature" either.

More documentation

We don't have to document every member function, but please do put a little write-up at the top of every header file. Assume that the reader is smart, unfamiliar with the code, and in a hurry. You can help them answer the questions "do I need to drill down into this?" and "how does this fit into the other code?"

One of my past roles involved a culture of exhaustive, strictly codified, soul crushing documentation standards, so it's unlikely you'll overdo it.

Even little member functions that seem obvious by name and context can benefit from a sentence saying what it's about.

Unit Tests

Your tests are good.

I'm not a fan of the REMOTE_CONFIG_TEST macro (I know, DRY), but it doesn't bother me too much. Besides, I used the preprocessor in environment.h, span_defaults.cpp, and version.cpp, so I'm unqualified to judge.

I looked at a recent code coverage report and there are some code sections that would be good to cover in tests if it's not too onerous:

• DatadogAgent::get_and_apply_remote_config_updates in datadog_agent.cpp
• a few branches in remote_config.cpp (this one might not be worth the trouble)

[dmehala]

Does it have to be a class with getter?

[dgoffredo]

Looking at the two (well, more than two) JSON payloads that use service, environment, and runtime_id, I saw that library language, library version, and library language version (all constants) were also there on an equal footing. Who's to say that those are not also part of the signature?

So, either I would keep it a struct having only the three fields, and leave the code that pulls the other values from the appropriate constants; or, I could wrap those constants into the type. I chose the latter, but am not committed to the idea.

I justified it to myself by saying "this makes the notion of a tracer signature more substantial."

[dmehala]

How about a third option where it's still a struct or class but with all fields accessible:

struct TracerSignature {
  RuntimeID runtime_id_;
  std::string default_service_;
  std::string default_environment_;
  StringView library_version = tracer_version;
  StringView library_language = "cpp";
  StringView library_version = __cplusplus;

  TracerSignature() = delete;
  TracerSignature(const RuntimeID& runtime_id, ...) : runtime_id(runtime_id) ... {}
};

[dgoffredo]

That works.

__cplusplus is an integer literal, though.

Options that I considered:

1. Just use std::string. That would work.
2. Use the preprocessor to stringify __cplusplus. This works, but then you get a trailing "L" due to the definition of the __cplusplus token. :(
3. Use member functions. :D

It also ever so slightly bothers me that library_* as data members implies that the values might mutate, when in actuality they never will. Maybe this is an indication that it's not such a good idea to put them in TraceSignature after all.

const data members aren't worth the trouble, seeing how they disable assignments and moves, though that might not matter for TracerSignature's usage.

Anything will do.

[dgoffredo]

Too cute? :P

https://godbolt.org/z/6xbY5c4Mf

[dmehala]

Perfect. Will replace substr with:

std::string_view version = std::string_view{STRINGIFY(__cplusplus), 6};

[dgoffredo]

std::string_view version = "most likely 201703";

[dmehala]

static_assert(__cplusplus == 201703L);
std::string_view version = "201703";

😈

if constexpr(__cplusplus == "199711") {
   constexpr std::string_view version = "199711";
} else if constexpr(__cplusplus == "201103") {
   constexpr std::string_view version = "201103";
}
...

I have no idea if it compiles.

[dgoffredo]

These are integrated into Damien's PR.