Skip to content

Commit 26350df

Browse files
fulmicotonfulmicoton
authored
Added a bash script to check the log format. (#4150)
1 parent 7a29318 commit 26350df

File tree

58 files changed

+164
-132
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

58 files changed

+164
-132
lines changed

‎CODE_STYLE.md

+15-4
Original file line numberDiff line numberDiff line change
@@ -109,15 +109,26 @@ These assert will not be part of the release binary and won't hurt the execution
109109

110110
**example needed**
111111

112-
## Errors
112+
## Errors and log messages
113113

114-
Error messages should be concise, lowercase (except proper names), and without trailing punctuation.
114+
Error and log messages follow the same format. They should be concise, lowercase (except proper names), and without trailing punctuation.
115115

116-
### Examples
116+
As a loose rule, where it does not hurt readability, log messages should rely on `tracing`
117+
structured logging instead of templating.
118+
119+
In other words, prefer:
120+
`warn!(remaining=remaining_attempts, "trubulizor rpc plane retry failed")`
121+
to
122+
`warn!("trubulizor rpc plane retry failed ({remaining_attempts} attempts remaining)")`
123+
124+
### Error Examples
117125
- "failed to start actor runtimes"
118126
- "cannot join PostgreSQL URI {} with path {:?}"
119127
- "could not find split metadata in Metastore {}"
120-
- "unkown output format {:?}"
128+
- "unknown output format {:?}"
129+
130+
### Log examples
131+
121132

122133
## Comments
123134

‎quickwit/Makefile

+2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ fmt:
66
@(rustup toolchain list | ( ! grep -q nightly && echo "Toolchain 'nightly' is not installed. Please install using 'rustup toolchain install nightly'.") ) || cargo +nightly fmt
77
@echo "Checking license headers"
88
@bash scripts/check_license_headers.sh
9+
@echo "Checking log format"
10+
@bash scripts/check_log_format.sh
911

1012
fix:
1113
@echo "Running cargo clippy --fix"

‎quickwit/quickwit-actors/src/lib.rs

+6-7
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,7 @@ pub use observation::{Observation, ObservationType};
6060
use quickwit_common::KillSwitch;
6161
pub use spawn_builder::SpawnContext;
6262
use thiserror::Error;
63-
use tracing::info;
64-
use tracing::log::warn;
63+
use tracing::{info, warn};
6564
pub use universe::Universe;
6665

6766
pub use self::actor_context::ActorContext;
@@ -93,19 +92,19 @@ fn heartbeat_from_env_or_default() -> Duration {
9392
match std::env::var("QW_ACTOR_HEARTBEAT_SECS") {
9493
Ok(actor_hearbeat_secs_str) => {
9594
if let Ok(actor_hearbeat_secs) = actor_hearbeat_secs_str.parse::<NonZeroU64>() {
96-
info!("Set the actor heartbeat to {actor_hearbeat_secs} seconds.");
95+
info!("set the actor heartbeat to {actor_hearbeat_secs} seconds");
9796
return Duration::from_secs(actor_hearbeat_secs.get());
9897
} else {
9998
warn!(
100-
"Failed to parse `QW_ACTOR_HEARTBEAT_SECS={actor_hearbeat_secs_str}` in \
101-
seconds > 0, using default heartbeat (30 seconds)."
99+
"failed to parse `QW_ACTOR_HEARTBEAT_SECS={actor_hearbeat_secs_str}` in \
100+
seconds > 0, using default heartbeat (30 seconds)"
102101
);
103102
};
104103
}
105104
Err(std::env::VarError::NotUnicode(os_str)) => {
106105
warn!(
107-
"Failed to parse `QW_ACTOR_HEARTBEAT_SECS={os_str:?}` in a valid unicode string, \
108-
using default heartbeat (30 seconds)."
106+
"failed to parse `QW_ACTOR_HEARTBEAT_SECS={os_str:?}` in a valid unicode string, \
107+
using default heartbeat (30 seconds)"
109108
);
110109
}
111110
Err(std::env::VarError::NotPresent) => {}

‎quickwit/quickwit-cli/src/jemalloc.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ pub async fn jemalloc_metrics_loop() -> tikv_jemalloc_ctl::Result<()> {
5959
pub fn start_jemalloc_metrics_loop() {
6060
tokio::task::spawn(async {
6161
if let Err(jemalloc_metrics_err) = jemalloc_metrics_loop().await {
62-
error!(err=?jemalloc_metrics_err, "Failed to gather metrics from jemalloc.");
62+
error!(err=?jemalloc_metrics_err, "failed to gather metrics from jemalloc");
6363
}
6464
});
6565
}

‎quickwit/quickwit-cli/src/lib.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ async fn load_node_config(config_uri: &Uri) -> anyhow::Result<NodeConfig> {
222222
let config = NodeConfig::load(config_format, config_content.as_slice())
223223
.await
224224
.with_context(|| format!("failed to parse node config `{config_uri}`"))?;
225-
info!(config_uri=%config_uri, config=?config, "Loaded node config.");
225+
info!(config_uri=%config_uri, config=?config, "loaded node config");
226226
Ok(config)
227227
}
228228

@@ -402,10 +402,10 @@ pub mod busy_detector {
402402

403403
let suppressed = SUPPRESSED_DEBUG_COUNT.swap(0, Ordering::Relaxed);
404404
if suppressed == 0 {
405-
debug!("Thread wasn't parked for {delta}µs, is the runtime too busy?");
405+
debug!("thread wasn't parked for {delta}µs, is the runtime too busy?");
406406
} else {
407407
debug!(
408-
"Thread wasn't parked for {delta}µs, is the runtime too busy? ({suppressed} \
408+
"thread wasn't parked for {delta}µs, is the runtime too busy? ({suppressed} \
409409
similar messages suppressed)"
410410
);
411411
}

‎quickwit/quickwit-cli/src/service.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ impl RunCliCommand {
8181
crate::busy_detector::set_enabled(true);
8282

8383
if let Some(services) = &self.services {
84-
tracing::info!(services = %services.iter().join(", "), "Setting services from override.");
84+
tracing::info!(services = %services.iter().join(", "), "setting services from override");
8585
node_config.enabled_services = services.clone();
8686
}
8787
let telemetry_handle_opt =

‎quickwit/quickwit-cli/src/tool.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -630,12 +630,12 @@ pub async fn merge_cli(args: MergeArgs) -> anyhow::Result<()> {
630630
let observation = pipeline_handle.last_observation();
631631

632632
if observation.num_ongoing_merges == 0 {
633-
info!("Merge pipeline has no more ongoing merges, Exiting.");
633+
info!("merge pipeline has no more ongoing merges, exiting");
634634
break;
635635
}
636636

637637
if pipeline_handle.state().is_exit() {
638-
info!("Merge pipeline has exited, Exiting.");
638+
info!("merge pipeline has exited, exiting");
639639
break;
640640
}
641641
}

‎quickwit/quickwit-cli/tests/helpers.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ impl TestEnv {
158158
};
159159
tokio::spawn(async move {
160160
if let Err(error) = run_command.execute().await {
161-
error!(err=?error, "Failed to start a quickwit server.");
161+
error!(err=?error, "failed to start a quickwit server");
162162
}
163163
});
164164
wait_for_server_ready(([127, 0, 0, 1], self.rest_listen_port).into()).await?;

‎quickwit/quickwit-cluster/src/member.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ fn parse_indexing_cpu_capacity(node_state: &NodeState) -> CpuCapacity {
122122
if let Ok(indexing_capacity) = CpuCapacity::from_str(indexing_capacity_str) {
123123
indexing_capacity
124124
} else {
125-
error!(indexing_capacity=?indexing_capacity_str, "Received an unparseable indexing capacity from node.");
125+
error!(indexing_capacity=?indexing_capacity_str, "received an unparseable indexing capacity from node");
126126
CpuCapacity::zero()
127127
}
128128
}

‎quickwit/quickwit-common/src/runtimes.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ impl RuntimeType {
120120
.get_or_init(|| {
121121
#[cfg(any(test, feature = "testsuite"))]
122122
{
123-
tracing::warn!("Starting Tokio actor runtimes for tests.");
123+
tracing::warn!("starting Tokio actor runtimes for tests");
124124
start_runtimes(RuntimesConfig::light_for_tests())
125125
}
126126
#[cfg(not(any(test, feature = "testsuite")))]

‎quickwit/quickwit-common/src/stream_utils.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ where T: Send + 'static
148148
Ok(Some(message)) => Some((message, streaming)),
149149
Ok(None) => None,
150150
Err(error) => {
151-
warn!(error=?error, "gRPC transport error.");
151+
warn!(error=?error, "gRPC transport error");
152152
None
153153
}
154154
}

‎quickwit/quickwit-config/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ impl ConfigFormat {
199199
serde_json::from_reader(StripComments::new(payload))?;
200200
let version_value = json_value.get_mut("version").context("missing version")?;
201201
if let Some(version_number) = version_value.as_u64() {
202-
warn!("`version` is supposed to be a string.");
202+
warn!(version_value=?version_value, "`version` is supposed to be a string");
203203
*version_value = JsonValue::String(version_number.to_string());
204204
}
205205
serde_json::from_value(json_value).context("failed to read JSON file")
@@ -211,7 +211,7 @@ impl ConfigFormat {
211211
toml::from_str(payload_str).context("failed to read TOML file")?;
212212
let version_value = toml_value.get_mut("version").context("missing version")?;
213213
if let Some(version_number) = version_value.as_integer() {
214-
warn!("`version` is supposed to be a string.");
214+
warn!(version_value=?version_value, "`version` is supposed to be a string");
215215
*version_value = toml::Value::String(version_number.to_string());
216216
let reserialized = toml::to_string(version_value)
217217
.context("failed to reserialize toml config")?;

‎quickwit/quickwit-config/src/node_config/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ impl NodeConfig {
353353
for peer_seed in &self.peer_seeds {
354354
let peer_seed_addr = HostAddr::parse_with_default_port(peer_seed, default_gossip_port)?;
355355
if let Err(error) = peer_seed_addr.resolve().await {
356-
warn!(peer_seed = %peer_seed_addr, error = ?error, "Failed to resolve peer seed address.");
356+
warn!(peer_seed = %peer_seed_addr, error = ?error, "failed to resolve peer seed address");
357357
continue;
358358
}
359359
peer_seed_addrs.push(peer_seed_addr.to_string())

‎quickwit/quickwit-config/src/node_config/serialize.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ fn default_node_id() -> ConfigValue<String, QW_NODE_ID> {
5252
Ok(short_hostname) => short_hostname,
5353
Err(error) => {
5454
let node_id = new_coolid("node");
55-
warn!(error=?error, "Failed to determine hostname or hostname was invalid, falling back to random node ID `{}`.", node_id);
55+
warn!(error=?error, "failed to determine hostname or hostname was invalid, falling back to random node ID `{}`", node_id);
5656
node_id
5757
}
5858
};
@@ -100,12 +100,12 @@ fn default_data_dir_uri() -> ConfigValue<Uri, QW_DATA_DIR> {
100100
fn default_advertise_host(listen_ip: &IpAddr) -> anyhow::Result<Host> {
101101
if listen_ip.is_unspecified() {
102102
if let Some((interface_name, private_ip)) = find_private_ip() {
103-
info!(advertise_address=%private_ip, interface_name=%interface_name, "Using sniffed advertise address.");
103+
info!(advertise_address=%private_ip, interface_name=%interface_name, "using sniffed advertise address");
104104
return Ok(Host::from(private_ip));
105105
}
106106
bail!("listen address `{listen_ip}` is unspecified and advertise address is not set");
107107
}
108-
info!(advertise_address=%listen_ip, "Using listen address as advertise address.");
108+
info!(advertise_address=%listen_ip, "using listen address as advertise address");
109109
Ok(Host::from(*listen_ip))
110110
}
111111

@@ -301,12 +301,12 @@ fn validate(node_config: &NodeConfig) -> anyhow::Result<()> {
301301

302302
if node_config.cluster_id == DEFAULT_CLUSTER_ID {
303303
warn!(
304-
"Cluster ID is not set, falling back to default value: `{}`.",
304+
"cluster ID is not set, falling back to default value: `{}`",
305305
DEFAULT_CLUSTER_ID
306306
);
307307
}
308308
if node_config.peer_seeds.is_empty() {
309-
warn!("Peer seed list is empty.");
309+
warn!("peer seed list is empty");
310310
}
311311
Ok(())
312312
}

‎quickwit/quickwit-control-plane/src/indexing_scheduler/mod.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ impl IndexingScheduler {
187187
crate::metrics::CONTROL_PLANE_METRICS.schedule_total.inc();
188188
let mut indexers: Vec<(String, IndexerNodeInfo)> = self.get_indexers_from_indexer_pool();
189189
if indexers.is_empty() {
190-
warn!("No indexer available, cannot schedule an indexing plan.");
190+
warn!("no indexer available, cannot schedule an indexing plan");
191191
return;
192192
};
193193

@@ -254,11 +254,11 @@ impl IndexingScheduler {
254254
last_applied_plan.indexing_tasks_per_indexer(),
255255
);
256256
if !indexing_plans_diff.has_same_nodes() {
257-
info!(plans_diff=?indexing_plans_diff, "Running plan and last applied plan node IDs differ: schedule an indexing plan.");
257+
info!(plans_diff=?indexing_plans_diff, "running plan and last applied plan node IDs differ: schedule an indexing plan");
258258
self.schedule_indexing_plan_if_needed(model);
259259
} else if !indexing_plans_diff.has_same_tasks() {
260260
// Some nodes may have not received their tasks, apply it again.
261-
info!(plans_diff=?indexing_plans_diff, "Running tasks and last applied tasks differ: reapply last plan.");
261+
info!(plans_diff=?indexing_plans_diff, "running tasks and last applied tasks differ: reapply last plan");
262262
self.apply_physical_indexing_plan(&mut indexers, last_applied_plan.clone());
263263
}
264264
}
@@ -272,7 +272,7 @@ impl IndexingScheduler {
272272
indexers: &mut [(String, IndexerNodeInfo)],
273273
new_physical_plan: PhysicalIndexingPlan,
274274
) {
275-
debug!("Apply physical indexing plan: {:?}", new_physical_plan);
275+
debug!(new_physical_plan=?new_physical_plan, "apply physical indexing plan");
276276
for (node_id, indexing_tasks) in new_physical_plan.indexing_tasks_per_indexer() {
277277
// We don't want to block on a slow indexer so we apply this change asynchronously
278278
// TODO not blocking is cool, but we need to make sure there is not accumulation
@@ -292,7 +292,7 @@ impl IndexingScheduler {
292292
.apply_indexing_plan(ApplyIndexingPlanRequest { indexing_tasks })
293293
.await
294294
{
295-
error!(indexer_node_id=%indexer.0, err=?error, "Error occurred when applying indexing plan to indexer.");
295+
error!(indexer_node_id=%indexer.0, err=?error, "error occurred when applying indexing plan to indexer");
296296
}
297297
}
298298
});

‎quickwit/quickwit-control-plane/src/indexing_scheduler/scheduling/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -508,7 +508,7 @@ pub fn build_physical_indexing_plan(
508508
// TODO this is probably a bad idea to just not overschedule, as having a single index trail
509509
// behind will prevent the log GC.
510510
// A better strategy would probably be to close shard, and start prevent ingestion.
511-
error!("unable to assign all sources in the cluster.");
511+
error!("unable to assign all sources in the cluster");
512512
}
513513

514514
// Convert the new scheduling solution back to a physical plan.

‎quickwit/quickwit-index-management/src/garbage_collection.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ async fn delete_splits_marked_for_deletion(
202202
.map(|split| split.split_metadata)
203203
.collect(),
204204
Err(error) => {
205-
error!(error = ?error, "Failed to fetch deletable splits.");
205+
error!(error = ?error, "failed to fetch deletable splits");
206206
break;
207207
}
208208
};

‎quickwit/quickwit-indexing/src/actors/indexing_pipeline.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -519,11 +519,11 @@ impl Handler<Spawn> for IndexingPipeline {
519519
if let Some(MetastoreError::NotFound { .. }) =
520520
spawn_error.downcast_ref::<MetastoreError>()
521521
{
522-
info!(error = ?spawn_error, "Could not spawn pipeline, index might have been deleted.");
522+
info!(error = ?spawn_error, "could not spawn pipeline, index might have been deleted");
523523
return Err(ActorExitStatus::Success);
524524
}
525525
let retry_delay = wait_duration_before_retry(spawn.retry_count + 1);
526-
error!(error = ?spawn_error, retry_count = spawn.retry_count, retry_delay = ?retry_delay, "Error while spawning indexing pipeline, retrying after some time.");
526+
error!(error = ?spawn_error, retry_count = spawn.retry_count, retry_delay = ?retry_delay, "error while spawning indexing pipeline, retrying after some time");
527527
ctx.schedule_self_msg(
528528
retry_delay,
529529
Spawn {

‎quickwit/quickwit-indexing/src/actors/indexing_service.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -579,11 +579,11 @@ impl IndexingService {
579579
)
580580
.await
581581
{
582-
error!(pipeline_id=?new_pipeline_id, err=?error, "Failed to spawn pipeline.");
582+
error!(pipeline_id=?new_pipeline_id, err=?error, "failed to spawn pipeline");
583583
failed_spawning_pipeline_ids.push(new_pipeline_id.clone());
584584
}
585585
} else {
586-
error!(pipeline_id=?new_pipeline_id, "Failed to spawn pipeline: source does not exist.");
586+
error!(pipeline_id=?new_pipeline_id, "failed to spawn pipeline: source does not exist");
587587
failed_spawning_pipeline_ids.push(new_pipeline_id.clone());
588588
}
589589
} else {
@@ -674,7 +674,7 @@ impl IndexingService {
674674
.queues
675675
.into_iter()
676676
.collect();
677-
debug!(queues=?queues, "List ingest API queues.");
677+
debug!(queues=?queues, "list ingest API queues");
678678

679679
let indexes_metadatas = self
680680
.metastore
@@ -686,7 +686,7 @@ impl IndexingService {
686686
.into_iter()
687687
.map(|index_metadata| index_metadata.index_id().to_string())
688688
.collect();
689-
debug!(index_ids=?index_ids, "List indexes.");
689+
debug!(index_ids=?index_ids, "list indexes");
690690

691691
let queue_ids_to_delete = queues.difference(&index_ids);
692692

‎quickwit/quickwit-indexing/src/actors/merge_pipeline.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -448,11 +448,11 @@ impl Handler<Spawn> for MergePipeline {
448448
if let Some(MetastoreError::NotFound { .. }) =
449449
spawn_error.downcast_ref::<MetastoreError>()
450450
{
451-
info!(error = ?spawn_error, "Could not spawn pipeline, index might have been deleted.");
451+
info!(error = ?spawn_error, "could not spawn pipeline, index might have been deleted");
452452
return Err(ActorExitStatus::Success);
453453
}
454454
let retry_delay = wait_duration_before_retry(spawn.retry_count);
455-
error!(error = ?spawn_error, retry_count = spawn.retry_count, retry_delay = ?retry_delay, "Error while spawning indexing pipeline, retrying after some time.");
455+
error!(error = ?spawn_error, retry_count = spawn.retry_count, retry_delay = ?retry_delay, "error while spawning indexing pipeline, retrying after some time");
456456
ctx.schedule_self_msg(
457457
retry_delay,
458458
Spawn {

‎quickwit/quickwit-indexing/src/actors/merge_planner.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ impl MergePlanner {
358358
// We run smaller merges in priority.
359359
merge_ops.sort_by_cached_key(|merge_op| Reverse(max_merge_ops(merge_op)));
360360
while let Some(merge_operation) = merge_ops.pop() {
361-
info!(merge_operation=?merge_operation, "Planned merge operation.");
361+
info!(merge_operation=?merge_operation, "planned merge operation");
362362
let tracked_merge_operation = self
363363
.ongoing_merge_operations_inventory
364364
.track(merge_operation);

‎quickwit/quickwit-indexing/src/actors/packager.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ fn create_packaged_split(
301301
append_to_tag_set(&named_field.name, &terms, &mut tags);
302302
}
303303
Err(tag_extraction_error) => {
304-
warn!(err=?tag_extraction_error, "No field values will be registered in the split metadata.");
304+
warn!(err=?tag_extraction_error, "no field values will be registered in the split metadata");
305305
}
306306
}
307307
}

‎quickwit/quickwit-indexing/src/actors/uploader.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ impl Handler<PackagedSplitBatch> for Uploader {
290290
let kill_switch = ctx.kill_switch().clone();
291291
let split_ids = batch.split_ids();
292292
if kill_switch.is_dead() {
293-
warn!(split_ids=?split_ids,"Kill switch was activated. Cancelling upload.");
293+
warn!(split_ids=?split_ids,"kill switch was activated, cancelling upload");
294294
return Err(ActorExitStatus::Killed);
295295
}
296296
let metastore = self.metastore.clone();
@@ -311,7 +311,7 @@ impl Handler<PackagedSplitBatch> for Uploader {
311311
for packaged_split in batch.splits.iter() {
312312
if batch.publish_lock.is_dead() {
313313
// TODO: Remove the junk right away?
314-
info!("Splits' publish lock is dead.");
314+
info!("splits' publish lock is dead");
315315
split_update_sender.discard()?;
316316
return Ok(());
317317
}

‎quickwit/quickwit-indexing/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ pub async fn start_indexing_service(
7575
storage_resolver: StorageResolver,
7676
event_broker: EventBroker,
7777
) -> anyhow::Result<Mailbox<IndexingService>> {
78-
info!("Starting indexer service.");
78+
info!("starting indexer service");
7979

8080
// Spawn indexing service.
8181
let indexing_service = IndexingService::new(

0 commit comments

Comments
 (0)