-
Notifications
You must be signed in to change notification settings - Fork 492
add node_id to emited spans #5985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,12 +19,12 @@ use std::collections::BTreeMap; | |
| use anyhow::Context; | ||
| use colored::Colorize; | ||
| use opentelemetry::global; | ||
| use quickwit_cli::busy_detector; | ||
| use quickwit_cli::checklist::RED_COLOR; | ||
| use quickwit_cli::cli::{CliCommand, build_cli}; | ||
| #[cfg(feature = "jemalloc")] | ||
| use quickwit_cli::jemalloc::start_jemalloc_metrics_loop; | ||
| use quickwit_cli::logger::setup_logging_and_tracing; | ||
| use quickwit_cli::{busy_detector, load_node_config}; | ||
| use quickwit_common::runtimes::scrape_tokio_runtime_metrics; | ||
| use quickwit_serve::BuildInfo; | ||
| use tracing::error; | ||
|
|
@@ -101,8 +101,28 @@ async fn main_impl() -> anyhow::Result<()> { | |
| start_jemalloc_metrics_loop(); | ||
|
|
||
| let build_info = BuildInfo::get(); | ||
| let env_filter_reload_fn = | ||
| setup_logging_and_tracing(command.default_log_level(), ansi_colors, build_info)?; | ||
|
|
||
| // we need this value very early to use it in otel, but this is so early we haven't setup | ||
| // logging yet. What we do is read the config, and if it fails, provide a default value. | ||
| // Except for race conditions, an error will get logged the 2nd time the config is read, | ||
| // inside the command. In case of race condition, we just don't know the node id for tracing | ||
| // purpose | ||
| let node_id = { | ||
| async || { | ||
| let config_uri = command.config_uri()?; | ||
| let config = load_node_config(config_uri).await.ok()?; | ||
| Some(config.node_id.take()) | ||
| } | ||
| }() | ||
| .await | ||
| .unwrap_or_else(|| "unknown".to_string()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the default value contain a random number in it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it won't be useful because the 2 situations where this is emitted should be:
but there is little harm at doing it, so why not |
||
|
|
||
| let env_filter_reload_fn = setup_logging_and_tracing( | ||
| command.default_log_level(), | ||
| ansi_colors, | ||
| build_info, | ||
| node_id, | ||
| )?; | ||
|
|
||
| let return_code: i32 = if let Err(command_error) = command.execute(env_filter_reload_fn).await { | ||
| error!(error=%command_error, "command failed"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I suggest we do simpler... but less correct.
Just use the
QW_NODE_IDenv variable here.It won't capture people using the
node_idconfig value, and it will break if we change the helm chart, but it has the merit of not adding the extra complexity in this PR (loading the config twice, etc.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(your call though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer reading the config: not all deployments of quickwit uses an helm chart
i'd argue that our configuration loading process is complexe, but well encapsulated, so loading the config twice doesn't harm code maintainability