remote config: more code review comments#83
remote config: more code review comments#83dgoffredo wants to merge 22 commits intodmehala/remote-config-implfrom
Conversation
…/dmehala/remote-config-impl
| namespace datadog { | ||
| namespace tracing { | ||
|
|
||
| class TracerSignature { |
There was a problem hiding this comment.
Does it have to be a class with getter?
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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) ... {}
};There was a problem hiding this comment.
That works.
__cplusplus is an integer literal, though.
Options that I considered:
- Just use
std::string. That would work. - Use the preprocessor to stringify
__cplusplus. This works, but then you get a trailing "L" due to the definition of the__cplusplustoken. :( - 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.
There was a problem hiding this comment.
Too cute? :P
There was a problem hiding this comment.
Perfect. Will replace substr with:
std::string_view version = std::string_view{STRINGIFY(__cplusplus), 6};There was a problem hiding this comment.
std::string_view version = "most likely 201703";There was a problem hiding this comment.
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.
|
These are integrated into Damien's PR. |
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:
class TracerInstead, 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 byTracerInfobut 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_TESTmacro (I know, DRY), but it doesn't bother me too much. Besides, I used the preprocessor inenvironment.h,span_defaults.cpp, andversion.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_updatesin datadog_agent.cpp