Replace panic calls with proper error handling by aaryan359 · Pull Request #7956 · jaegertracing/jaeger

aaryan359 · 2026-02-01T20:21:52Z

This PR replaces several panic() calls with proper error handling across Jaeger CLI tools, improving robustness and preventing unexpected runtime crashes.

Which problem is this PR solving?

Resolves [chore]: Replace panic() calls with proper error handling #7955

Description of the changes

Replaced panic(err) with error returns in InitFromViper methods
- cmd/es-rollover/app/flags.go
- cmd/es-index-cleaner/app/flags.go
Updated ParseJaegerTags to return an error for invalid tag pairs
- cmd/internal/flags/flags.go
Updated all callers to properly handle returned errors
- cmd/es-index-cleaner/main.go
- cmd/es-rollover/app/actions.go
Updated and extended tests to assert error conditions instead of panics
- cmd/es-rollover/app/flags_test.go
- cmd/es-index-cleaner/app/flags_test.go
- cmd/internal/flags/flags_test.go

These changes align with Go best practices and improve CLI stability without changing functionality.

How was this change tested?

make fmt
make lint
make test

All tests pass successfully.

Checklist

I have read the contributing guidelines
I have signed all commits
I have added/updated unit tests where applicable
I have run lint and test steps successfully

- Replace panic(err) with error returns in InitFromViper methods - cmd/es-rollover/app/flags.go: Return error from InitFromViper - cmd/es-index-cleaner/app/flags.go: Return error from InitFromViper - Replace panic with error return in ParseJaegerTags - cmd/internal/flags/flags.go: Return error for invalid tag pairs - Replace panic with proper error handling in tracegen - cmd/tracegen/main.go: Handle logger creation error gracefully - Update all callers to handle returned errors - cmd/es-index-cleaner/main.go: Check InitFromViper error - cmd/es-rollover/app/actions.go: Check InitFromViper error - cmd/es-rollover/app/flags_test.go: Update test assertions - cmd/es-index-cleaner/app/flags_test.go: Update test assertions - cmd/internal/flags/flags_test.go: Update tests for error handling This improves error handling and prevents runtime crashes by properly propagating errors instead of panicking. Signed-off-by: aaryan359 <[email protected]>

dosubot · 2026-02-01T20:22:01Z

Related Documentation

No published documentation to review for changes on this repository.

Write your first living document

^{How did I do? Any feedback?}

Signed-off-by: aaryan359 <[email protected]>

aaryan359 · 2026-02-01T21:20:18Z

@yurishkuro,
I’ve updated the tests as suggested and restored the panic in tracegen/main.go.
Please let me know if there’s anything else to tweak.

codecov · 2026-02-01T22:18:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.58%. Comparing base (2d4e012) to head (d3ed4df).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7956   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files         316      316           
  Lines       16763    16766    +3     
=======================================
+ Hits        16023    16026    +3     
- Misses        578      579    +1     
+ Partials      162      161    -1

Flag	Coverage Δ
badger_v1	`9.16% <ø> (ø)`
badger_v2	`1.89% <ø> (ø)`
cassandra-4.x-v1-manual	`13.36% <ø> (ø)`
cassandra-4.x-v2-auto	`1.88% <ø> (ø)`
cassandra-4.x-v2-manual	`1.88% <ø> (ø)`
cassandra-5.x-v1-manual	`13.36% <ø> (ø)`
cassandra-5.x-v2-auto	`1.88% <ø> (ø)`
cassandra-5.x-v2-manual	`1.88% <ø> (ø)`
clickhouse	`1.97% <ø> (ø)`
elasticsearch-6.x-v1	`17.20% <ø> (ø)`
elasticsearch-7.x-v1	`17.23% <ø> (ø)`
elasticsearch-8.x-v1	`17.38% <ø> (ø)`
elasticsearch-8.x-v2	`1.89% <ø> (ø)`
elasticsearch-9.x-v2	`1.89% <ø> (ø)`
grpc_v1	`8.41% <ø> (ø)`
grpc_v2	`1.89% <ø> (ø)`
kafka-3.x-v2	`1.89% <ø> (ø)`
memory_v2	`1.89% <ø> (ø)`
opensearch-1.x-v1	`17.27% <ø> (ø)`
opensearch-2.x-v1	`17.27% <ø> (ø)`
opensearch-2.x-v2	`1.89% <ø> (ø)`
opensearch-3.x-v2	`1.89% <ø> (ø)`
query	`1.89% <ø> (ø)`
tailsampling-processor	`0.54% <ø> (ø)`
unittests	`94.27% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

aaryan359 · 2026-02-02T07:52:08Z

@yurishkuro thanks for the review!
I noticed a small coverage change due to the new error-handling paths — let me know if you’d like me to add tests for those cases, happy to do so.

Copilot

Pull request overview

This PR improves CLI robustness by removing panic()-based error handling and replacing it with returned/propagated errors that can be handled gracefully by callers.

Changes:

Updated ParseJaegerTags to return (map[string]string, error) and added tests for invalid tag inputs.
Updated es-rollover and es-index-cleaner configs to return errors from InitFromViper instead of panicking, and updated callers/tests to handle these errors.
Adjusted tests to assert error behavior rather than panic behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
cmd/internal/flags/flags.go	`ParseJaegerTags` now returns an error instead of panicking on invalid input.
cmd/internal/flags/flags_test.go	Updated tests to validate error returns and added invalid-input coverage.
cmd/es-rollover/app/flags.go	`Config.InitFromViper` now returns an error instead of panicking on TLS init failure.
cmd/es-rollover/app/actions.go	Propagates `InitFromViper` errors with context instead of assuming no-fail initialization.
cmd/es-rollover/app/flags_test.go	Updated to assert `InitFromViper` succeeds via `require.NoError`.
cmd/es-index-cleaner/app/flags.go	`Config.InitFromViper` now returns an error instead of panicking on TLS init failure.
cmd/es-index-cleaner/main.go	Caller now handles `InitFromViper` errors and returns them from the command.
cmd/es-index-cleaner/app/flags_test.go	Updated to assert `InitFromViper` succeeds via `require.NoError`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-02T13:36:53Z

+func ParseJaegerTags(jaegerTags string) (map[string]string, error) {
 	if jaegerTags == "" {
-		return nil
+		return nil, nil
 	}


PR description states that cmd/tracegen/main.go was updated to replace the logger-creation panic with graceful stderr reporting, but that change is not present in this PR’s diffs. Either include the cmd/tracegen/main.go update (it currently still panics on logger build failure) or adjust the PR description/scope so it matches the actual changes.

Signed-off-by: aaryan359 <[email protected]>

aaryan359 · 2026-02-02T14:35:21Z

@jkowall, now i fixed the test coverage and fixed the description also . can this meged now

Signed-off-by: aaryan359 <[email protected]>

This PR replaces several `panic()` calls with proper error handling across Jaeger CLI tools, improving robustness and preventing unexpected runtime crashes. ## Which problem is this PR solving? - Resolves jaegertracing#7955 ## Description of the changes - Replaced `panic(err)` with error returns in `InitFromViper` methods - `cmd/es-rollover/app/flags.go` - `cmd/es-index-cleaner/app/flags.go` - Updated `ParseJaegerTags` to return an error for invalid tag pairs - `cmd/internal/flags/flags.go` - Updated all callers to properly handle returned errors - `cmd/es-index-cleaner/main.go` - `cmd/es-rollover/app/actions.go` - Updated and extended tests to assert error conditions instead of panics - `cmd/es-rollover/app/flags_test.go` - `cmd/es-index-cleaner/app/flags_test.go` - `cmd/internal/flags/flags_test.go` These changes align with Go best practices and improve CLI stability without changing functionality. ## How was this change tested? - `make fmt` - `make lint` - `make test` All tests pass successfully. ## Checklist - [x] I have read the contributing guidelines - [x] I have signed all commits - [x] I have added/updated unit tests where applicable - [x] I have run lint and test steps successfully --------- Signed-off-by: aaryan359 <[email protected]>

aaryan359 requested a review from a team as a code owner February 1, 2026 20:21

dosubot Bot added the go Pull requests that update go code label Feb 1, 2026

yurishkuro requested changes Feb 1, 2026

View reviewed changes

Comment thread cmd/es-rollover/app/flags_test.go Outdated

Comment thread cmd/es-index-cleaner/app/flags_test.go Outdated

Comment thread cmd/tracegen/main.go

update tests and keep panic in tracegen main

1350ac5

Signed-off-by: aaryan359 <[email protected]>

yurishkuro approved these changes Feb 1, 2026

View reviewed changes

yurishkuro added changelog:bugfix-or-minor-feature changelog:refactoring Internal code refactoring without functional changes and removed changelog:bugfix-or-minor-feature labels Feb 1, 2026

yurishkuro enabled auto-merge February 1, 2026 22:11

jkowall requested a review from Copilot February 2, 2026 13:27

Copilot started reviewing on behalf of jkowall February 2, 2026 13:27 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

increase test coverage

a3ba79e

Signed-off-by: aaryan359 <[email protected]>

auto-merge was automatically disabled February 2, 2026 13:48
Head branch was pushed to by a user without write access

yurishkuro approved these changes Feb 2, 2026

View reviewed changes

imporove test coverage for action_test file

d3ed4df

Signed-off-by: aaryan359 <[email protected]>

yurishkuro approved these changes Feb 3, 2026

View reviewed changes

yurishkuro enabled auto-merge February 3, 2026 03:38

yurishkuro added this pull request to the merge queue Feb 3, 2026

Merged via the queue into jaegertracing:main with commit dc94e83 Feb 3, 2026
61 checks passed

aaryan359 deleted the fix/replace-panic-with-error-handling branch April 8, 2026 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace panic calls with proper error handling#7956

Replace panic calls with proper error handling#7956
yurishkuro merged 4 commits into
jaegertracing:mainfrom
aaryan359:fix/replace-panic-with-error-handling

aaryan359 commented Feb 1, 2026 •

edited

Loading

Uh oh!

dosubot Bot commented Feb 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaryan359 commented Feb 1, 2026

Uh oh!

codecov Bot commented Feb 1, 2026 •

edited

Loading

Uh oh!

aaryan359 commented Feb 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

aaryan359 commented Feb 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aaryan359 commented Feb 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

dosubot Bot commented Feb 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaryan359 commented Feb 1, 2026

Uh oh!

codecov Bot commented Feb 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aaryan359 commented Feb 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

aaryan359 commented Feb 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaryan359 commented Feb 1, 2026 •

edited

Loading

codecov Bot commented Feb 1, 2026 •

edited

Loading