-
Notifications
You must be signed in to change notification settings - Fork 64
OTA-1974: Teach Cincinnati to include product information when serving /graph-data #1073
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: master
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 |
|---|---|---|
|
|
@@ -65,6 +65,7 @@ async fn main() -> Result<(), Error> { | |
| let live = Arc::new(RwLock::new(false)); | ||
| let ready = Arc::new(RwLock::new(false)); | ||
| let secondary_metadata = Arc::new(RwLock::new(String::new())); | ||
| let product_data_path = Arc::new(RwLock::new(None)); | ||
| graph::State::new( | ||
| json_graph, | ||
| settings.mandatory_client_parameters.clone(), | ||
|
|
@@ -73,9 +74,16 @@ async fn main() -> Result<(), Error> { | |
| Box::leak(Box::new(plugins)), | ||
| Box::leak(Box::new(registry)), | ||
| secondary_metadata, | ||
| product_data_path, | ||
| ) | ||
| }; | ||
|
|
||
| // Extract product lifecycle settings before moving settings | ||
| let product_enabled = settings.product_enabled; | ||
| let product_api_url = settings.product_api_url.clone(); | ||
| let product_poll_interval_secs = settings.product_poll_interval_secs; | ||
| let product_timeout_secs = settings.product_timeout_secs; | ||
|
|
||
| // Graph scraper | ||
| { | ||
| let graph_state = state.clone(); | ||
|
|
@@ -84,8 +92,25 @@ async fn main() -> Result<(), Error> { | |
| }); | ||
| } | ||
|
|
||
| // Product lifecycle fetcher | ||
| if product_enabled { | ||
| let product_state = Arc::new(state.clone()); | ||
| tokio::spawn(async move { | ||
|
Member
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. Comparing with a few lines up where we're using
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. My understanding here is that since this is an async function, |
||
| graph_builder::product_lifecycle::run( | ||
| product_api_url, | ||
| product_poll_interval_secs, | ||
| product_timeout_secs, | ||
| product_state, | ||
| ) | ||
| .await | ||
| }); | ||
| } else { | ||
| info!("Product lifecycle fetching is disabled"); | ||
| } | ||
|
|
||
| // Status service. | ||
| graph::register_metrics(state.registry())?; | ||
| graph_builder::product_lifecycle::register_metrics(state.registry())?; | ||
|
|
||
| let status_state = state.clone(); | ||
| let metrics_server = HttpServer::new(move || { | ||
|
|
@@ -212,6 +237,7 @@ mod tests { | |
| metrics::new_registry(Some(config::METRICS_PREFIX.to_string())).unwrap(), | ||
| )); | ||
| let secondary_metadata = Arc::new(RwLock::new(String::new())); | ||
| let product_data_path = Arc::new(RwLock::new(None)); | ||
|
|
||
| State::new( | ||
| json_graph, | ||
|
|
@@ -221,6 +247,7 @@ mod tests { | |
| plugins, | ||
| registry, | ||
| secondary_metadata, | ||
| product_data_path, | ||
| ) | ||
| } | ||
|
|
||
|
|
||
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.
What's going on with this change away from
chrono? We've been usingchronosince 43f39e8 (#136), and while I like being able to use the stdlib and have one fewer dep to keep track of, I'd like to see some notes in the commit message explaining what this change is about. Was the use of a non-stdlib dep unneccessary from the start? Or has the stdlib grown functionality that obsoleted a previously-valid need for a dep? Or is this actually changing user-visible behavior? Or...?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.
Hi! It doesn't change anything for the user, both these outputs produce the same Unix timestamp value.
I don't have any strong opinions on this at all. Do you think it's best that it's kept as it is rather than removing it?