Update events guide#1987
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1987 +/- ##
==========================================
- Coverage 85.78% 85.78% -0.01%
==========================================
Files 513 513
Lines 163122 163122
==========================================
- Hits 139941 139929 -12
- Misses 22647 22659 +12
Partials 534 534
🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
ci-repo
| - `%value` -- Display formatting (`fmt::Display`) | ||
| - `?value` -- Debug formatting (`fmt::Debug`) | ||
| - `value` -- passed directly (integers, booleans, etc.) | ||
|
|
||
| ```rust | ||
| otel_info!("node.connect", | ||
| endpoint = %addr, | ||
| config = ?node_config, | ||
| count = 42, | ||
| ); | ||
| ``` |
| echo "" | ||
| echo " See docs/telemetry/events-guide.md for full details." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Should we add a test that there are no space characters in the event name? (Is that a weaver thing?)
(Can it be checked in the macro?)
There was a problem hiding this comment.
Yes we need to enforce as much as possible in compile time, and then validate against published schema in CI time (via weaver).
This PR is one baby step towards that Goal.
| @@ -0,0 +1,111 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Will this script catch the case where someone uses the macro in a way where, for example, the case tracing::warn! doesn't actually appear in the file? e.g.:
use tracing::warn;
// ...
warn!("...");Would it be easier or more effective to just use the clippy rule for banned macros? We already do this for lazy_static:
otel-arrow/rust/otap-dataflow/.clippy.toml
Lines 17 to 19 in 466fa02
There was a problem hiding this comment.
I started with clippy for this, but clippy wasn't able to distinguish "the developer wrote tracing::info!" from "the developer wrote otel_info! which expanded to tracing::info!". The script here was my cheap way to do some enforcement, while I can find better ways!
lquerel
left a comment
lquerel
left a comment
There was a problem hiding this comment.
I really like the update on the documentation.
I also agree with Albert, using Clippy to detect and reject tokio tracing macros might be better.
|
I suggest we merge and improve clippy settings in a future PR. Thank you @cijothomas! |
# Change Summary Applied suggestion from #1987 ## Are there any user-facing changes? Yes, less expensive logs, without losing information!
4 participants
Guidance updated on proper usage of Events (AKA Structured Logs with Durable Name)
Enforce at CI any direct usage of tokio/tracing or log macros.