-
Notifications
You must be signed in to change notification settings - Fork 94
Update events guide #1987
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
Update events guide #1987
Changes from all commits
5143a17
e645958
45cd3c5
15f545d
29821e8
edb062c
bc53fea
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 | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,111 @@ | ||||||||
| #!/bin/bash | ||||||||
|
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. Will this script catch the case where someone uses the macro in a way where, for example, the case 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
Member
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 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! |
||||||||
| # Script to detect direct use of tracing/log macros instead of otel_* wrappers. | ||||||||
| # All events MUST be emitted using the otel_* macros from otap_df_telemetry. | ||||||||
| # See docs/telemetry/events-guide.md | ||||||||
| # | ||||||||
| # Usage: ./scripts/check-direct-telemetry-macros.sh | ||||||||
|
|
||||||||
| set -e | ||||||||
|
|
||||||||
| echo "🔍 Checking for direct use of tracing/log macros (should use otel_* macros instead)..." | ||||||||
|
|
||||||||
| # Function to check for a banned pattern in .rs files | ||||||||
| check_banned_pattern() { | ||||||||
| local pattern="$1" | ||||||||
| local description="$2" | ||||||||
| local files | ||||||||
|
|
||||||||
| # Find all .rs files, excluding: | ||||||||
| # - target/ build artifacts | ||||||||
| # - .git/ git internals | ||||||||
| # - crates/telemetry/ implements the otel_* wrappers (legitimate tracing usage) | ||||||||
| # - crates/quiver/ will move out of this repo (temporary exemption) | ||||||||
| # - crates/quiver-e2e/ quiver end-to-end tests (temporary exemption) | ||||||||
| # - benchmarks/ benchmarks may legitimately use tracing directly | ||||||||
| files=$(find . -name "*.rs" -type f \ | ||||||||
| | grep -v target/ \ | ||||||||
| | grep -v .git/ \ | ||||||||
| | grep -v crates/telemetry/ \ | ||||||||
| | grep -v crates/quiver/ \ | ||||||||
| | grep -v crates/quiver-e2e/ \ | ||||||||
| | grep -v benchmarks/ \ | ||||||||
| || true) | ||||||||
|
|
||||||||
| if [ -z "$files" ]; then | ||||||||
| return 0 | ||||||||
| fi | ||||||||
|
|
||||||||
| local matches | ||||||||
| matches=$(echo "$files" | xargs grep -n "$pattern" 2>/dev/null || true) | ||||||||
|
|
||||||||
| if [ -n "$matches" ]; then | ||||||||
| echo "⚠️ Found direct $description:" | ||||||||
| echo "$matches" | ||||||||
| echo "" | ||||||||
| return 1 | ||||||||
| fi | ||||||||
| return 0 | ||||||||
| } | ||||||||
|
|
||||||||
| checks_passed=0 | ||||||||
|
|
||||||||
| # Check for fully-qualified tracing macro calls | ||||||||
| if ! check_banned_pattern "tracing::info!" "tracing::info! usage (use otel_info! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "tracing::warn!" "tracing::warn! usage (use otel_warn! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "tracing::error!" "tracing::error! usage (use otel_error! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "tracing::debug!" "tracing::debug! usage (use otel_debug! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "tracing::trace!" "tracing::trace! usage (use otel_debug! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| # Check for fully-qualified log crate macro calls | ||||||||
| if ! check_banned_pattern "log::info!" "log::info! usage (use otel_info! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "log::warn!" "log::warn! usage (use otel_warn! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "log::error!" "log::error! usage (use otel_error! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "log::debug!" "log::debug! usage (use otel_debug! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if ! check_banned_pattern "log::trace!" "log::trace! usage (use otel_debug! instead)"; then | ||||||||
| checks_passed=1 | ||||||||
| fi | ||||||||
|
|
||||||||
| if [ $checks_passed -eq 0 ]; then | ||||||||
| echo "✅ No direct tracing/log macro usage found!" | ||||||||
| echo "ℹ️ All events use otel_* macros as required by docs/telemetry/events-guide.md." | ||||||||
| else | ||||||||
| echo "❌ Found direct tracing/log macro usage. Use otel_* macros from otap_df_telemetry instead." | ||||||||
| echo "" | ||||||||
| echo "How to fix:" | ||||||||
| echo " • tracing::info!(...) → otel_info!(\"event.name\", ...)" | ||||||||
| echo " • tracing::warn!(...) → otel_warn!(\"event.name\", ...)" | ||||||||
| echo " • tracing::error!(...) → otel_error!(\"event.name\", ...)" | ||||||||
| echo " • tracing::debug!(...) → otel_debug!(\"event.name\", ...)" | ||||||||
| echo "" | ||||||||
| echo " Import from otap_df_telemetry:" | ||||||||
| echo " use otap_df_telemetry::otel_info;" | ||||||||
| echo "" | ||||||||
| echo " See docs/telemetry/events-guide.md for full details." | ||||||||
| exit 1 | ||||||||
| fi | ||||||||
|
Contributor
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 we add a test that there are no space characters in the event name? (Is that a weaver thing?)
Member
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. 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. |
||||||||
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.
Thanks!