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

Support logging in starlark #8408

Merged
merged 1 commit into from
Nov 16, 2020
Merged

Support logging in starlark #8408

merged 1 commit into from
Nov 16, 2020

Conversation

essobedo
Copy link
Contributor

Required for all PRs:

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

Fix for #8402

Motivation

Logging primitives to tie into the Telegraf logger are missing

Modifications:

  • Adds a new module to manage all the logging primitives
  • Uses a static logger to log the message as there is no easy way to use the current one

@essobedo essobedo changed the title Support logging Support logging in starlark Nov 14, 2020
@srebhan srebhan self-assigned this Nov 16, 2020
@srebhan
Copy link
Contributor

srebhan commented Nov 16, 2020

While your changes look good, I'm a bit hesitant to add this to to telegraf as IMO this should really go to starlark itself. Otherwise we are basically forking starlark... I've e.g. some time/duration module in the pipe and could also add this, but this means that we bloat telegraf by stuff that actually belongs to starlark.
@ssoroka what is your opinion on this?

@ssoroka
Copy link
Contributor

ssoroka commented Nov 16, 2020

@srebhan This is what I was expecting. I don't see anything specific to logging in Starlark, and even so it'd need some tie in to our logger. This does sort of bring us to a situation where we have a sort of sub-dialect of Starlark, but I don't necessarily see that as a problem.

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

looks great.

@srebhan
Copy link
Contributor

srebhan commented Nov 16, 2020

@ssoroka: Well you could easily define an interface for the logger and allow to specify this when initializing the logging module... IF we want to go the route of adding all that stuff from InfluxData side, we should actually fork starklark and add all that modules in the fork. This way we will not clutter this plugin. In the end, the real problem is where to stop. I've a date/time/duration module, someone else will come up with a math module and the next guy submits a timeseries prediction... Where do you cut this? Again, using those modules in this plugin is not my concern, just plugging more and more stuff in here...

@essobedo this is no objection to your code (quite the contrary), IMO we need to get this straight now as otherwise it is hard to stop somewhere.

@ssoroka
Copy link
Contributor

ssoroka commented Nov 16, 2020

Going to merge and then pull out the module to https://github.com/influxdata/starlark-go-modules afterwards.

Thanks for the PR @essobedo ❤️

@ssoroka ssoroka merged commit 0c15569 into influxdata:master Nov 16, 2020
@essobedo essobedo deleted the 8402/logging branch November 16, 2020 20:26
@essobedo
Copy link
Contributor Author

@ssoroka Is there any issue you want me to look at in particular?

@ssoroka
Copy link
Contributor

ssoroka commented Nov 16, 2020

@essobedo find me on the community slack for detailed conversation... but maybe you could post your ideas on #8371 and start thinking about that. Possible options are multiple return values, or maybe if there are problems with that an exposed method to add new metrics.

@essobedo
Copy link
Contributor Author

@ssoroka ok, I can look at #8371, you can assign it to, I will do it asap

ssoroka pushed a commit that referenced this pull request Dec 1, 2020
(cherry picked from commit 0c15569)
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants