Skip to content
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

Allow to manage errors that occur in the apply function #8401

Merged
merged 1 commit into from
Nov 13, 2020
Merged

Allow to manage errors that occur in the apply function #8401

merged 1 commit into from
Nov 13, 2020

Conversation

essobedo
Copy link
Contributor

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Fix for #8354

Motivation

There is no way to manage error returned in the apply function.

Modifications:

  • Adds a new built-in function called catch based on the function of the same name of starlarktest
  • Adds a new test to ensure that it works as expected
  • Adds a new sub section of Common Questions into the README.md to describe how to use it

@ssoroka
Copy link
Contributor

ssoroka commented Nov 13, 2020

I like where you're going with this. I'm guessing in this case you specifically don't want the error to prevent the metric from being parsed? or are you just trying to make the error visible?

@essobedo
Copy link
Contributor Author

essobedo commented Nov 13, 2020

@ssoroka

I like where you're going with this. I'm guessing in this case you specifically don't want the error to prevent the metric from being parsed? or are you just trying to make the error visible?

I would answer both, I want to have the full control if an error occurs. But initially my use case was indeed to ensure that the metric is processed in case of an error. With the current code, if you have an error, the metric is lost while I would like to keep it and add the error message to it.

@ssoroka
Copy link
Contributor

ssoroka commented Nov 13, 2020

I agree the user should have full control over what happens. I was thinking there should be some logging primitives, too, to tie into the Telegraf logger. something like log.info, log.warn, log.error, and log.debug, so you can customize what to log around error conditions.

I'd also like the ability to explicitly return an error. I believe starlark has a built-in fail() command that can do this, but I haven't tested it.

@essobedo
Copy link
Contributor Author

essobedo commented Nov 13, 2020

@ssoroka

I'd also like the ability to explicitly return an error. I believe starlark has a built-in fail() command that can do this, but I haven't tested it.

Yes the fail function is part starlarktest and is for testing purpose. It actually leverages the (same) catch function as you can see here

@ssoroka
Copy link
Contributor

ssoroka commented Nov 13, 2020

Have any interest in adding the other functions I mentioned in another PR? I've got a list of outstanding starlark issues here, and I'd be more than happy for the assistance. area/starlark

@ssoroka ssoroka merged commit ca04106 into influxdata:master Nov 13, 2020
@ssoroka
Copy link
Contributor

ssoroka commented Nov 13, 2020

also feel free to join the #Telegraf channel on InfluxData slack, and consider writing starlark-based tests in the future as they get executed and double as documentation

@essobedo essobedo deleted the 8354/manage_error branch November 13, 2020 19:37
@essobedo
Copy link
Contributor Author

Have any interest in adding the other functions I mentioned in another PR? I've got a list of outstanding starlark issues here, and I'd be more than happy for the assistance. area/starlark

Thanks for proposing, yes I'm interested in helping you if I can.

I will try to propose a PR asap for the logging, I guess it is #8402

@ssoroka
Copy link
Contributor

ssoroka commented Nov 13, 2020

feel free to add to that issue. it's mostly just a placeholder to attach work and descriptions to later.

ssoroka pushed a commit that referenced this pull request Dec 1, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants