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

[pkg/telemetryquerylanguage] Add the ability to use a logger in functions #13544

Merged
merged 8 commits into from
Sep 13, 2022

Conversation

evan-bradley
Copy link
Contributor

@evan-bradley evan-bradley commented Aug 23, 2022

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.

@evan-bradley evan-bradley requested a review from a team August 23, 2022 22:06
@evan-bradley evan-bradley force-pushed the issue-9730-update-parser branch 2 times, most recently from 7c58322 to 49f158e Compare August 23, 2022 22:17
@TylerHelmuth
Copy link
Member

@evan-bradley Can you check the failing tests? I'll do a review tomorrow.

@evan-bradley evan-bradley force-pushed the issue-9730-update-parser branch 2 times, most recently from 2e5126f to f92837f Compare August 24, 2022 14:18
Copy link
Member

@bogdandrutu bogdandrutu left a 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.

@bogdandrutu bogdandrutu dismissed their stale review August 25, 2022 00:09

Did not mean to block, but we need to make sure we get this right.

@TylerHelmuth
Copy link
Member

Pass "TelemetrySettings" not logger, since same argument can be done to record metrics.

@bogdandrutu can you elaborate? Are you saying we should be able to pass the entire TelemetrySettings struct into functions?

@evan-bradley evan-bradley force-pushed the issue-9730-update-parser branch 5 times, most recently from a709d40 to 9cbab83 Compare August 26, 2022 23:43
Copy link
Member

@TylerHelmuth TylerHelmuth left a 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.

pkg/telemetryquerylanguage/tql/README.md Outdated Show resolved Hide resolved
Copy link
Member

@TylerHelmuth TylerHelmuth left a 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.

@TylerHelmuth
Copy link
Member

@evan-bradley please address CI failures.

@evan-bradley
Copy link
Contributor Author

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.

@dmitryax
Copy link
Member

@evan-bradley PTAL #13753 to fix the linter issue

@dmitryax
Copy link
Member

dmitryax commented Sep 7, 2022

@evan-bradley please rebase

@evan-bradley
Copy link
Contributor Author

@dmitryax Everything should be in order now.

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.

6 participants