review of dmehala/remote-config-impl (#74) #76
Conversation
BenchmarksBenchmark execution time: 2023-12-07 19:38:44 Comparing candidate commit 06aace8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
| @@ -138,18 +149,16 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) { | |||
|
|
|||
| const auto& config_metadata = | |||
| targets.at("/signed/targets"_json_pointer).at(config_path); | |||
| if (!contains(config_path, k_apm_product) || | |||
| if (!contains(config_path, k_apm_product_path_substring) || | |||
There was a problem hiding this comment.
Idea from Caleb: Write a partial parser that reads the first path component, and based on its value either retrieves the appropriate component, or returns some "n/a" value.
| decoder.buffer = 0 | c0 << (32 - 6 * 1) | c1 << (32 - 6 * 2) | | ||
| c2 << (32 - 6 * 3) | c3 << (32 - 6 * 4); |
There was a problem hiding this comment.
I find the extensive use of arithmetic not helpful, it does the opposite. Ultimately, the reader is required to understand the base64 algorithm to grasp the rationale behind those bit shifting.
Either static_cast<uint32_t>(c0) << 26 or c0 << (32 - 6 * 1), is equivalent but the later require mental calculation to find out how much we are shifting, add on top of it a lack of parenthesis and I guarantee lots of people will fail to interpret it correctly compared to << 26. Can we avoid this confusion and revert to the proposed solution?
There was a problem hiding this comment.
You can write it however you prefer it, but my preference is expressed in the diff.
More important to me is that we choose an algorithm that does not assume little-endian.
| struct Update { | ||
| Optional<TraceSamplerConfig> trace_sampler; | ||
| }; |
There was a problem hiding this comment.
An Update is capable of being consumed by the ConfigManager, even though it is likely and perhaps exclusively intended to be consumed by the ConfigManager, that does not means there is a compelling reason to tighly couple both concept. It can also be interpreted as an Update of the ConfigManager, which is not the intended meaning.
I suggest to keep it outside of the ConfigManager, it offers room for flexibility, probably future usage and semantically is better.
There was a problem hiding this comment.
I saw three options:
- Leave it as you had it.
- Put
struct ConfigUpdatein its own component (its own.h). Good place for documentation! - Consider
ConfigUpdateas part ofConfigManager's public interface.
(1) and (3) are compatible, but so too are (3) and my proposed change.
My reasoning was "if I see ConfigUpdate used in RemoteConfigManager, where do I go looking for its definition?" I might look for a config_update.h, but there is none. ConfigUpdate is a parameter to ConfigManager, so I might look there, but the connection is even stronger if it's spelled ConfigManager::Update.
It can also be interpreted as an
Updateof theConfigManager, which is not the intended meaning.
That's a good point. I prefer (2) to (1) if you're looking for a compromise. I'm also fine with (1). This PR is illustrative only.
There was a problem hiding this comment.
Thanks for taking a look. I left some responses as inline comments.
Thoughts:
- If you disagree with my style suggestions, go with your gut. This is your rodeo.
- Please see my comment mentioning "Idea from Caleb." This is related to the griping I did on slack yesterday about the config path concept.
- One opinion that I do hold strongly is endian agnosticism. Please choose an alternative base64 decoding algorithm. I won't be picky about the style.
| decoder.buffer = 0 | c0 << (32 - 6 * 1) | c1 << (32 - 6 * 2) | | ||
| c2 << (32 - 6 * 3) | c3 << (32 - 6 * 4); |
There was a problem hiding this comment.
You can write it however you prefer it, but my preference is expressed in the diff.
More important to me is that we choose an algorithm that does not assume little-endian.
| struct Update { | ||
| Optional<TraceSamplerConfig> trace_sampler; | ||
| }; |
There was a problem hiding this comment.
I saw three options:
- Leave it as you had it.
- Put
struct ConfigUpdatein its own component (its own.h). Good place for documentation! - Consider
ConfigUpdateas part ofConfigManager's public interface.
(1) and (3) are compatible, but so too are (3) and my proposed change.
My reasoning was "if I see ConfigUpdate used in RemoteConfigManager, where do I go looking for its definition?" I might look for a config_update.h, but there is none. ConfigUpdate is a parameter to ConfigManager, so I might look there, but the connection is even stronger if it's spelled ConfigManager::Update.
It can also be interpreted as an
Updateof theConfigManager, which is not the intended meaning.
That's a good point. I prefer (2) to (1) if you're looking for a compromise. I'm also fine with (1). This PR is illustrative only.
Each commit in this pull request is effectively a review comment for #74.
This pull request is a work in progress.
So far, my suggestions are all stylistic.
Please note specifically any
TODO(@dgoffredo)comments that I've added.