-
-
Notifications
You must be signed in to change notification settings - Fork 39
feat(logs): Improve logs with spans #194
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
feat(logs): Improve logs with spans #194
Conversation
f831319
to
241b81e
Compare
api/Cargo.toml
Outdated
@@ -45,7 +45,7 @@ sqlx = { workspace = true, features = [ | |||
thiserror = { workspace = true } | |||
tokio = { workspace = true, features = ["rt-multi-thread", "macros"] } | |||
tracing = { workspace = true, default-features = false } | |||
tracing-actix-web = { workspace = true, features = ["emit_event_on_error"] } |
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.
We emit errors on our own.
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.
Left a couple of comments.
let apply_worker_span = tracing::info_span!( | ||
"apply_worker", | ||
pipeline_id = self.pipeline_id, | ||
publication_name = self.config.publication_name |
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.
Are pipeline_id
and publication_name
useful for every log line within the span? There's going to only one of each while the replicator is running. As opposed to something like a table_id
which will be different.
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.
My idea was that we want a reliable way to get a view of all logs of a certain pipeline for debugging a single customer.
@@ -145,7 +145,7 @@ async fn init_state_store( | |||
Ok(PostgresStateStore::new(pipeline_id, pg_connection_config)) | |||
} | |||
|
|||
#[instrument(skip(pipeline))] | |||
#[tracing::instrument(skip(pipeline), fields(pipeline_id = pipeline.id()))] |
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.
Same here, is pipeline_id
needed?
This PR improves logging in several ways: