Skip to content

Commit ee82a39

Browse files
author
Andrei Popescu
authored
Reduce excesive logging in API repositories. (#848)
The SDK logs to much and to often especially in the API repositories. This commit removes some of the duplicated logs, aligns all logs to a common structure and adds additional info like HRN and key to all important logs. All other logs that are not important are now on level debug. All previous trace logs which are actually printing debug level information are now raised to debug level. One very important fix that needs highlight is the capture by copy which was removed from most of the cache repositories while putting into cache. It is not necessary to capture model data by copy as the KeyValueCache::Put() is pure sync and will not leave the context so that the model faces deletion. This additional copy is not needed and might even bring performance decreases. This is now fixed and all models are captured by reference when added to cache. Additionally this commit also enables ccplint namespace check. Resolves: OLPSUP-10501 Signed-off-by: Andrei Popescu <andrei.popescu@here.com>
1 parent d634d53 commit ee82a39

File tree

10 files changed

+318
-247
lines changed

10 files changed

+318
-247
lines changed

‎olp-cpp-sdk-dataservice-read/src/ApiClientLookup.cpp‎

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -59,39 +59,45 @@ ApiClientLookup::ApiClientResponse ApiClientLookup::LookupApi(
5959
std::string service_version, FetchOptions options,
6060
client::OlpClientSettings settings) {
6161
repository::ApiCacheRepository repository(catalog, settings.cache);
62+
const auto hrn = catalog.ToCatalogHRNString();
6263

6364
if (options != OnlineOnly && options != CacheWithUpdate) {
6465
auto url = repository.Get(service, service_version);
6566
if (url) {
66-
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApi(%s, %s) -> from cache",
67-
service.c_str(), service_version.c_str());
67+
OLP_SDK_LOG_DEBUG_F(kLogTag, "LookupApi(%s/%s) found in cache, hrn='%s'",
68+
service.c_str(), service_version.c_str(),
69+
hrn.c_str());
6870
client::OlpClient client;
6971
client.SetSettings(std::move(settings));
7072
client.SetBaseUrl(*url);
7173
return client;
7274
} else if (options == CacheOnly) {
73-
return client::ApiError(
74-
client::ErrorCode::NotFound,
75-
"Cache only resource not found in cache (loopup api).");
75+
return {{client::ErrorCode::NotFound,
76+
"CacheOnly: resource not found in cache"}};
7677
}
7778
}
7879

80+
OLP_SDK_LOG_INFO_F(kLogTag,
81+
"LookupApi(%s/%s) cache miss, requesting, hrn='%s'",
82+
service.c_str(), service_version.c_str(), hrn.c_str());
83+
7984
const auto& base_url = GetDatastoreServerUrl(catalog.partition);
8085
client::OlpClient client;
8186
client.SetBaseUrl(base_url);
87+
// Do not move settings, we still need them later on!
8288
client.SetSettings(settings);
8389
PlatformApi::ApisResponse api_response;
90+
8491
if (service == "config") {
8592
api_response = PlatformApi::GetApis(client, cancellation_context);
8693
} else {
87-
api_response = ResourcesApi::GetApis(client, catalog.ToCatalogHRNString(),
88-
cancellation_context);
94+
api_response = ResourcesApi::GetApis(client, hrn, cancellation_context);
8995
}
9096

9197
if (!api_response.IsSuccessful()) {
92-
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApi(%s/%s): %s - unsuccessful: %s",
93-
service.c_str(), service_version.c_str(),
94-
catalog.partition.c_str(),
98+
OLP_SDK_LOG_INFO_F(kLogTag,
99+
"LookupApi(%s/%s) unsuccessful, hrn='%s', error='%s'",
100+
service.c_str(), service_version.c_str(), hrn.c_str(),
95101
api_response.GetError().GetMessage().c_str());
96102
return api_response.GetError();
97103
}
@@ -106,24 +112,25 @@ ApiClientLookup::ApiClientResponse ApiClientLookup::LookupApi(
106112

107113
auto it =
108114
std::find_if(api_result.begin(), api_result.end(),
109-
[&](const olp::dataservice::read::model::Api& obj) -> bool {
115+
[&](const dataservice::read::model::Api& obj) -> bool {
110116
return (obj.GetApi().compare(service) == 0 &&
111117
obj.GetVersion().compare(service_version) == 0);
112118
});
113119
if (it == api_result.end()) {
114-
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApi(%s/%s): %s - service not available",
115-
service.c_str(), service_version.c_str(),
116-
catalog.partition.c_str());
120+
OLP_SDK_LOG_WARNING_F(
121+
kLogTag, "LookupApi(%s/%s) service not found, hrn='%s'",
122+
service.c_str(), service_version.c_str(), hrn.c_str());
117123

118-
return client::ApiError(client::ErrorCode::ServiceUnavailable,
119-
"Service/Version not available for given HRN");
124+
return {{client::ErrorCode::ServiceUnavailable,
125+
"Service/Version not available for given HRN"}};
120126
}
121127

122128
const auto& service_url = it->GetBaseUrl();
123129

124-
OLP_SDK_LOG_INFO_F(kLogTag, "LookupApi(%s/%s): %s - OK, service_url=%s",
125-
service.c_str(), service_version.c_str(),
126-
catalog.partition.c_str(), service_url.c_str());
130+
OLP_SDK_LOG_DEBUG_F(kLogTag,
131+
"LookupApi(%s/%s) found, hrn='%s', service_url='%s'",
132+
service.c_str(), service_version.c_str(), hrn.c_str(),
133+
service_url.c_str());
127134

128135
client::OlpClient service_client;
129136
service_client.SetBaseUrl(service_url);

‎olp-cpp-sdk-dataservice-read/src/repositories/ApiCacheRepository.cpp‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
namespace {
2626
constexpr auto kLogTag = "ApiCacheRepository";
27+
constexpr time_t kLookupApiExpiryTime = 3600;
2728

2829
std::string CreateKey(const std::string& hrn, const std::string& service,
2930
const std::string& serviceVersion) {
@@ -35,26 +36,26 @@ namespace olp {
3536
namespace dataservice {
3637
namespace read {
3738
namespace repository {
38-
using namespace olp::client;
3939
ApiCacheRepository::ApiCacheRepository(
40-
const HRN& hrn, std::shared_ptr<cache::KeyValueCache> cache)
40+
const client::HRN& hrn, std::shared_ptr<cache::KeyValueCache> cache)
4141
: hrn_(hrn), cache_(cache) {}
4242

4343
void ApiCacheRepository::Put(const std::string& service,
44-
const std::string& serviceVersion,
45-
const std::string& serviceUrl) {
44+
const std::string& version,
45+
const std::string& url) {
4646
std::string hrn(hrn_.ToCatalogHRNString());
47-
auto key = CreateKey(hrn, service, serviceVersion);
48-
OLP_SDK_LOG_TRACE_F(kLogTag, "Put '%s'", key.c_str());
49-
cache_->Put(CreateKey(hrn, service, serviceVersion), serviceUrl,
50-
[serviceUrl]() { return serviceUrl; }, 3600);
47+
auto key = CreateKey(hrn, service, version);
48+
OLP_SDK_LOG_DEBUG_F(kLogTag, "Put -> '%s'", key.c_str());
49+
50+
cache_->Put(key, url, [&]() { return url; }, kLookupApiExpiryTime);
5151
}
5252

5353
boost::optional<std::string> ApiCacheRepository::Get(
54-
const std::string& service, const std::string& serviceVersion) {
54+
const std::string& service, const std::string& version) {
5555
std::string hrn(hrn_.ToCatalogHRNString());
56-
auto key = CreateKey(hrn, service, serviceVersion);
57-
OLP_SDK_LOG_TRACE_F(kLogTag, "Get '%s'", key.c_str());
56+
auto key = CreateKey(hrn, service, version);
57+
OLP_SDK_LOG_DEBUG_F(kLogTag, "Get -> '%s'", key.c_str());
58+
5859
auto url = cache_->Get(key, [](const std::string& value) { return value; });
5960
if (url.empty()) {
6061
return boost::none;

‎olp-cpp-sdk-dataservice-read/src/repositories/CatalogCacheRepository.cpp‎

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ constexpr auto kLogTag = "CatalogCacheRepository";
3838

3939
// Currently, we expire the catalog version after 5 minutes. Later we plan to
4040
// give the user the control when to expire it.
41-
constexpr auto kCatalogVersionExpireTime = 5 * 60;
41+
constexpr auto kCatalogVersionExpiryTime = 5 * 60;
4242
constexpr auto kChronoSecondsMax = std::chrono::seconds::max();
4343
constexpr auto kTimetMax = std::numeric_limits<time_t>::max();
4444

@@ -56,61 +56,65 @@ namespace olp {
5656
namespace dataservice {
5757
namespace read {
5858
namespace repository {
59-
using namespace olp::client;
6059
CatalogCacheRepository::CatalogCacheRepository(
61-
const HRN& hrn, std::shared_ptr<cache::KeyValueCache> cache,
60+
const client::HRN& hrn, std::shared_ptr<cache::KeyValueCache> cache,
6261
std::chrono::seconds default_expiry)
6362
: hrn_(hrn), cache_(cache), default_expiry_(ConvertTime(default_expiry)) {}
6463

6564
void CatalogCacheRepository::Put(const model::Catalog& catalog) {
6665
std::string hrn(hrn_.ToCatalogHRNString());
6766
auto key = CreateKey(hrn);
68-
OLP_SDK_LOG_TRACE_F(kLogTag, "Put '%s'", key.c_str());
67+
OLP_SDK_LOG_DEBUG_F(kLogTag, "Put -> '%s'", key.c_str());
68+
6969
cache_->Put(key, catalog,
70-
[catalog]() { return olp::serializer::serialize(catalog); },
70+
[&]() { return olp::serializer::serialize(catalog); },
7171
default_expiry_);
7272
}
7373

7474
boost::optional<model::Catalog> CatalogCacheRepository::Get() {
7575
std::string hrn(hrn_.ToCatalogHRNString());
7676
auto key = CreateKey(hrn);
77-
OLP_SDK_LOG_TRACE_F(kLogTag, "Get '%s'", key.c_str());
78-
auto cachedCatalog =
79-
cache_->Get(key, [](const std::string& serializedObject) {
80-
return parser::parse<model::Catalog>(serializedObject);
81-
});
82-
if (cachedCatalog.empty()) {
77+
OLP_SDK_LOG_DEBUG_F(kLogTag, "Get -> '%s'", key.c_str());
78+
79+
auto cached_catalog = cache_->Get(key, [](const std::string& value) {
80+
return parser::parse<model::Catalog>(value);
81+
});
82+
83+
if (cached_catalog.empty()) {
8384
return boost::none;
8485
}
8586

86-
return boost::any_cast<model::Catalog>(cachedCatalog);
87+
return boost::any_cast<model::Catalog>(cached_catalog);
8788
}
8889

8990
void CatalogCacheRepository::PutVersion(const model::VersionResponse& version) {
9091
std::string hrn(hrn_.ToCatalogHRNString());
91-
OLP_SDK_LOG_TRACE_F(kLogTag, "PutVersion '%s'", hrn.c_str());
92+
OLP_SDK_LOG_DEBUG_F(kLogTag, "PutVersion -> '%s'", hrn.c_str());
93+
9294
cache_->Put(VersionKey(hrn), version,
93-
[version]() { return olp::serializer::serialize(version); },
94-
kCatalogVersionExpireTime);
95+
[&]() { return olp::serializer::serialize(version); },
96+
kCatalogVersionExpiryTime);
9597
}
9698

9799
boost::optional<model::VersionResponse> CatalogCacheRepository::GetVersion() {
98100
std::string hrn(hrn_.ToCatalogHRNString());
99101
auto key = VersionKey(hrn);
100-
OLP_SDK_LOG_TRACE_F(kLogTag, "GetVersion '%s'", key.c_str());
101-
auto cachedVersion =
102-
cache_->Get(key, [](const std::string& serializedObject) {
103-
return parser::parse<model::VersionResponse>(serializedObject);
104-
});
105-
if (cachedVersion.empty()) {
102+
OLP_SDK_LOG_DEBUG_F(kLogTag, "GetVersion -> '%s'", key.c_str());
103+
104+
auto cached_version = cache_->Get(key, [](const std::string& value) {
105+
return parser::parse<model::VersionResponse>(value);
106+
});
107+
108+
if (cached_version.empty()) {
106109
return boost::none;
107110
}
108-
return boost::any_cast<model::VersionResponse>(cachedVersion);
111+
return boost::any_cast<model::VersionResponse>(cached_version);
109112
}
110113

111114
void CatalogCacheRepository::Clear() {
112115
std::string hrn(hrn_.ToCatalogHRNString());
113-
OLP_SDK_LOG_TRACE_F(kLogTag, "Clear '%s'", CreateKey(hrn).c_str());
116+
OLP_SDK_LOG_INFO_F(kLogTag, "Clear -> '%s'", CreateKey(hrn).c_str());
117+
114118
cache_->RemoveKeysWithPrefix(hrn);
115119
}
116120

‎olp-cpp-sdk-dataservice-read/src/repositories/CatalogRepository.cpp‎

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
*/
1919

2020
#include "CatalogRepository.h"
21+
2122
#include <iostream>
23+
#include <utility>
2224

2325
#include <olp/core/client/CancellationContext.h>
2426
#include <olp/core/client/Condition.h>
@@ -32,40 +34,38 @@
3234
#include "olp/dataservice/read/CatalogRequest.h"
3335
#include "olp/dataservice/read/CatalogVersionRequest.h"
3436

37+
namespace {
38+
constexpr auto kLogTag = "CatalogRepository";
39+
} // namespace
40+
3541
namespace olp {
3642
namespace dataservice {
3743
namespace read {
3844
namespace repository {
3945

40-
namespace {
41-
constexpr auto kLogTag = "CatalogRepository";
42-
43-
} // namespace
44-
4546
CatalogResponse CatalogRepository::GetCatalog(
4647
client::HRN catalog, client::CancellationContext cancellation_context,
4748
CatalogRequest request, client::OlpClientSettings settings) {
48-
using namespace client;
49-
5049
const auto request_key = request.CreateKey();
5150
const auto fetch_options = request.GetFetchOption();
5251

53-
OLP_SDK_LOG_TRACE_F(kLogTag, "getCatalog '%s'", request_key.c_str());
54-
5552
repository::CatalogCacheRepository repository{
5653
catalog, settings.cache, settings.default_cache_expiration};
5754

5855
if (fetch_options != OnlineOnly && fetch_options != CacheWithUpdate) {
5956
auto cached = repository.Get();
6057
if (cached) {
61-
OLP_SDK_LOG_INFO_F(kLogTag, "cache catalog '%s' found!",
62-
request_key.c_str());
58+
OLP_SDK_LOG_DEBUG_F(
59+
kLogTag, "GetCatalog found in cache, hrn='%s', key='%s'",
60+
catalog.ToCatalogHRNString().c_str(), request_key.c_str());
61+
6362
return *cached;
6463
} else if (fetch_options == CacheOnly) {
65-
OLP_SDK_LOG_INFO_F(kLogTag, "cache catalog '%s' not found!",
66-
request_key.c_str());
67-
return ApiError(ErrorCode::NotFound,
68-
"Cache only resource not found in cache (catalog).");
64+
OLP_SDK_LOG_INFO_F(
65+
kLogTag, "GetCatalog not found in cache, hrn='%s', key='%s'",
66+
catalog.ToCatalogHRNString().c_str(), request_key.c_str());
67+
return {{client::ErrorCode::NotFound,
68+
"CacheOnly: resource not found in cache"}};
6969
}
7070
}
7171

@@ -76,7 +76,7 @@ CatalogResponse CatalogRepository::GetCatalog(
7676
return config_api.GetError();
7777
}
7878

79-
const OlpClient& client = config_api.GetResult();
79+
const client::OlpClient& client = config_api.GetResult();
8080
auto catalog_response =
8181
ConfigApi::GetCatalog(client, catalog.ToCatalogHRNString(),
8282
request.GetBillingTag(), cancellation_context);
@@ -86,6 +86,11 @@ CatalogResponse CatalogRepository::GetCatalog(
8686
if (!catalog_response.IsSuccessful()) {
8787
const auto& error = catalog_response.GetError();
8888
if (error.GetHttpStatusCode() == http::HttpStatusCode::FORBIDDEN) {
89+
OLP_SDK_LOG_WARNING_F(kLogTag,
90+
"GetCatalog 403 received, remove from cache, "
91+
"hrn='%s', key='%s'",
92+
catalog.ToCatalogHRNString().c_str(),
93+
request_key.c_str());
8994
repository.Clear();
9095
}
9196
}
@@ -96,23 +101,22 @@ CatalogResponse CatalogRepository::GetCatalog(
96101
CatalogVersionResponse CatalogRepository::GetLatestVersion(
97102
client::HRN catalog, client::CancellationContext cancellation_context,
98103
CatalogVersionRequest request, client::OlpClientSettings settings) {
99-
using namespace client;
100-
101104
repository::CatalogCacheRepository repository(catalog, settings.cache);
102105

103106
auto fetch_option = request.GetFetchOption();
104107
if (fetch_option != OnlineOnly && fetch_option != CacheWithUpdate) {
105108
auto cached_version = repository.GetVersion();
106109
if (cached_version) {
107-
OLP_SDK_LOG_INFO_F(kLogTag, "cache catalog '%s' found!",
108-
request.CreateKey().c_str());
110+
OLP_SDK_LOG_DEBUG_F(
111+
kLogTag, "GetLatestVersion found in cache, hrn='%s', key='%s'",
112+
catalog.ToCatalogHRNString().c_str(), request.CreateKey().c_str());
109113
return cached_version.value();
110114
} else if (fetch_option == CacheOnly) {
111-
OLP_SDK_LOG_INFO_F(kLogTag, "cache catalog '%s' not found!",
112-
request.CreateKey().c_str());
113-
return ApiError(
114-
ErrorCode::NotFound,
115-
"Cache only resource not found in cache (catalog version).");
115+
OLP_SDK_LOG_INFO_F(
116+
kLogTag, "GetLatestVersion not found in cache, hrn='%s', key='%s'",
117+
catalog.ToCatalogHRNString().c_str(), request.CreateKey().c_str());
118+
return {{client::ErrorCode::NotFound,
119+
"CacheOnly: resource not found in cache"}};
116120
}
117121
}
118122

@@ -132,9 +136,15 @@ CatalogVersionResponse CatalogRepository::GetLatestVersion(
132136
if (version_response.IsSuccessful() && fetch_option != OnlineOnly) {
133137
repository.PutVersion(version_response.GetResult());
134138
}
139+
135140
if (!version_response.IsSuccessful()) {
136141
const auto& error = version_response.GetError();
137142
if (error.GetHttpStatusCode() == http::HttpStatusCode::FORBIDDEN) {
143+
OLP_SDK_LOG_WARNING_F(kLogTag,
144+
"GetLatestVersion 403 received, remove from cache, "
145+
"hrn='%s', key='%s'",
146+
catalog.ToCatalogHRNString().c_str(),
147+
request.CreateKey().c_str());
138148
repository.Clear();
139149
}
140150
}

0 commit comments

Comments
 (0)