Skip to content

Fail when JQ filter fails#894

Merged
lquerel merged 7 commits into
open-telemetry:mainfrom
lmolkova:fail-on-jq-filter-errors
Aug 22, 2025
Merged

Fail when JQ filter fails#894
lquerel merged 7 commits into
open-telemetry:mainfrom
lmolkova:fail-on-jq-filter-errors

Conversation

@lmolkova

@lmolkova lmolkova commented Aug 21, 2025

Copy link
Copy Markdown
Member

We're only failing when filter is invalid, but ignore execution errors.

Fixed it, added test, and debug logs

Before: no output, no error, complete black hole

After:

image

@lmolkova
lmolkova requested a review from a team as a code owner August 21, 2025 20:06
Comment thread crates/weaver_forge/src/jq.rs
Comment thread crates/weaver_forge/src/jq.rs Outdated
Comment thread crates/weaver_forge/src/jq.rs Fixed
Comment thread crates/weaver_forge/src/jq.rs Fixed
Comment thread crates/weaver_forge/src/jq.rs Fixed
Comment thread crates/weaver_forge/src/jq.rs Fixed
Comment thread defaults/jq/semconv.jq
@codecov

codecov Bot commented Aug 21, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.9%. Comparing base (7beff49) to head (565b098).

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #894     +/-   ##
=======================================
- Coverage   77.9%   77.9%   -0.1%     
=======================================
  Files         76      76             
  Lines       5956    5967     +11     
=======================================
+ Hits        4643    4650      +7     
- Misses      1313    1317      +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lmolkova
lmolkova requested a review from jsuereth August 22, 2025 00:07
Comment thread docs/usage.md

Options:
--debug... Turn debugging information on
--debug... Turn debugging information on. Use twice (--debug --debug) for trace-level logs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird. Why not have --trace?

@lmolkova lmolkova Aug 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was following existing format:

weaver/src/cli.rs

Lines 21 to 22 in 7beff49

#[arg(long, action = clap::ArgAction::Count, global = true)]
pub debug: u8,

and was wondering if there is a convention. I realized it's similar to what curl does with -v, -vv, -vvv. Curious how @lquerel envisioned it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was to be able to define the verbosity level. It’s common practice to have -v, -vv, -vvv, but I didn’t specify that I wanted the short -v and not -d. Sorry for this oversight. If you prefer to use --trace, I have no objection.

@lquerel

lquerel commented Aug 22, 2025

Copy link
Copy Markdown
Contributor

Thanks for this fix. It will greatly improve the user experience!

@lquerel
lquerel merged commit cf8ef86 into open-telemetry:main Aug 22, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants