Skip to content

Fail when JQ filter fails#894

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

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

Conversation

@lmolkova
Copy link
Member

@lmolkova lmolkova commented Aug 21, 2025

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
@codecov
Copy link

codecov bot commented Aug 21, 2025

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

Options:
--debug... Turn debugging information on
--debug... Turn debugging information on. Use twice (--debug --debug) for trace-level logs.
Copy link
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?

Copy link
Member Author

@lmolkova lmolkova Aug 22, 2025

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
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
Copy link
Contributor

lquerel commented Aug 22, 2025

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.

4 participants