-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[pkg/telemetryquerylanguage] Add the ability to use a logger in functions #13544
[pkg/telemetryquerylanguage] Add the ability to use a logger in functions #13544
Conversation
7c58322
to
49f158e
Compare
@evan-bradley Can you check the failing tests? I'll do a review tomorrow. |
2e5126f
to
f92837f
Compare
7d24b7d
to
769397b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass "TelemetrySettings" not logger, since same argument can be done to record metrics.
Did not mean to block, but we need to make sure we get this right.
@bogdandrutu can you elaborate? Are you saying we should be able to pass the entire TelemetrySettings struct into functions? |
a709d40
to
9cbab83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evan-bradley I like the approach you've taken to introduce Logger. I think this is a great starting point towards refactoring the TQL to be more usable. @bogdandrutu I am still curious to hear more about your thoughts on passing the entire TelemetrySettings object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this implementation of Logger as it continues to allow the TQL to be generic to any telemetry, not only within the context of the Collector. If we want to pass in something like TelemetrySettings we could, but I don't think we would wrap it like this PR does for logger; it would be the literal TelemetrySettings type.
That change would be another step towards making the TQL specific to the collector, which I think is a conversation larger than this PR.
@evan-bradley please address CI failures. |
8fc46d9
to
fd3cefd
Compare
Unfortunately I don't think the CI failures are related to any changes I've made, the errors call out deprecated function usage in some exporters. I rebased on the latest commit on main, but that doesn't appear to have solved it. |
@evan-bradley PTAL #13753 to fix the linter issue |
fd3cefd
to
d3b7b19
Compare
@evan-bradley please rebase |
d3b7b19
to
d535de3
Compare
This makes it easier to pass context between functions
d535de3
to
c9fb777
Compare
@dmitryax Everything should be in order now. |
Description:
I've changed the TQL parsing functions to be methods on a struct that holds all common information necessary for parsing. Processors now instantiate the struct with a logger, which can then be injected into functions.
Link to tracking Issue:
Prerequisite for #9730.
Testing:
Unit tests were added, and manual tests were performed using modified functions.
Documentation:
I've added a note in the TQL readme aimed at function authors for how to get a logger.