Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(agent): Avoid panic by checking for skip_processors_after_aggregators #16477

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Feb 4, 2025

Summary

Check for the option not being set and issue a warning just as we do for normal runs...

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #16399

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Feb 4, 2025
@srebhan srebhan self-assigned this Feb 4, 2025
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 4, 2025

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 5, 2025
@srebhan srebhan assigned DStrand1 and unassigned srebhan Feb 5, 2025
@DStrand1 DStrand1 merged commit d00ee7c into influxdata:master Feb 5, 2025
27 checks passed
@github-actions github-actions bot added this to the v1.33.2 milestone Feb 5, 2025
@Hipska
Copy link
Contributor

Hipska commented Feb 6, 2025

I feel like this check should have been places somewhere else, because now the same code snippet now already exists 3 times in the codebase, and maybe probably another place to put it is forgotten.

Config.LoadAll would seem a good place to me.

I would also take into account the quiet CLI argument, and frankly the message itself doesn't make much sense in here, since you just want to check if a config file is valid or not. And wether you set this config item, the config is still valid.

srebhan added a commit that referenced this pull request Feb 10, 2025
@Hipska
Copy link
Contributor

Hipska commented Feb 10, 2025

@srebhan

@srebhan
Copy link
Member Author

srebhan commented Feb 18, 2025

@Hipska it's not possible to put this somewhere else as e.g. in LoadAll the logging is not yet set up. As this is supposed to go away with a later version anyway I think it's okay to have it duplicated... For the quiet option, it should already be honored by the logging framework. If that's not the case, please open a new issue!

@Hipska
Copy link
Contributor

Hipska commented Feb 18, 2025

Logging framework? You are calling log.Print, not the telegraf logging framework.

asaharn pushed a commit to asaharn/telegraf that referenced this pull request Mar 10, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/agent fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config check results in panic
3 participants