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

[processor/transform] Move config structure to TQL #13373

Merged
merged 6 commits into from
Aug 24, 2022

Conversation

TylerHelmuth
Copy link
Member

Description:
Moves the basic SQL-like config to config to the TQL. This will allow users of the TQL to use the SQL-like config.

This is a building-block to providing a declarative-like configuration.

Link to tracking Issue:
Works towards #11852

Testing:
Updated unit tests

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 15, 2022
@TylerHelmuth TylerHelmuth requested a review from a team August 15, 2022 21:57
@TylerHelmuth
Copy link
Member Author

/cc @kentquirk

}

// SignalConfig configures TQL queries to execute.
type SignalConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever want to have a different config between signals?

Copy link
Member Author

Choose a reason for hiding this comment

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

The current config allows configuring different queries per signal, but thats it so far. Haven't come across a use-case yet for the config beside defining the queries themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you thinking instead of a SignalConfig we'd have TracesConfig, MetricsConfig, and LogsConfig? At the moment all 3 would need the Queries property and would be duplicates of one another. Have anything in mind that we'd add to one but not the other?

Copy link
Member

Choose a reason for hiding this comment

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

Should we consider having each of Traces, Metrics, and Logs be an interface instead of a struct? That would be something like:

type Queryer interface {
    GetQueries() []string
}

Or does that screw up something relating to loading the configurations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe they need to be structs to interact correctly with component.ProcessorFactory

@TylerHelmuth TylerHelmuth requested a review from kentquirk August 16, 2022 16:15
@TylerHelmuth
Copy link
Member Author

@kentquirk @bogdandrutu please review.

Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

One question, but if it's not a good idea then this seems fine.

}

// SignalConfig configures TQL queries to execute.
type SignalConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider having each of Traces, Metrics, and Logs be an interface instead of a struct? That would be something like:

type Queryer interface {
    GetQueries() []string
}

Or does that screw up something relating to loading the configurations?

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Aug 24, 2022
@bogdandrutu bogdandrutu merged commit 74012c1 into open-telemetry:main Aug 24, 2022
@TylerHelmuth TylerHelmuth deleted the issue-11852-move-config branch August 24, 2022 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants