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

feat: allow user to convert datatype if valid #585

Merged
merged 14 commits into from
Jan 31, 2023

Conversation

fchikwekwe
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • This change allows a data check to the rules condition to see if input span/trace data matches the type specified on the data condition. If it is not, then attempt to convert the data to specified type (e.g. int to float, string to int, etc). If it cannot be converted, then do not convert the data.

@fchikwekwe fchikwekwe requested a review from a team January 10, 2023 16:17
@fchikwekwe fchikwekwe changed the title Faith/rules convert datatypes faith/rules-convert-datatypes Jan 10, 2023
@fchikwekwe fchikwekwe changed the title faith/rules-convert-datatypes feat: allow user to convert datatype if valid Jan 10, 2023
Copy link
Contributor

@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.

@fchikwekwe left a couple questions and a couple nits. Great work!

config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Outdated Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Outdated Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Outdated Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
sample/rules.go Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
@fchikwekwe fchikwekwe requested a review from a team as a code owner January 25, 2023 18:12
@fchikwekwe fchikwekwe force-pushed the faith/rules-convert-datatypes branch from df64c34 to 437b3ed Compare January 25, 2023 18:41
Copy link
Contributor

@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.

It's getting close. Needs more testing.

config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Outdated Show resolved Hide resolved
config/sampler_config.go Show resolved Hide resolved
config/sampler_config.go Outdated Show resolved Hide resolved
sample/rules_test.go Show resolved Hide resolved
@fchikwekwe fchikwekwe force-pushed the faith/rules-convert-datatypes branch from 75d743e to d116409 Compare January 31, 2023 15:04
Copy link
Contributor

@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.

Left 1 nit, otherwise LGTM.

if !conditionMatchesValue(condition, value, exists) {
ruleMatched = false
break // if any condition fails, we can't possibly succeed, so exit inner loop early
if condition.Matches != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make it here we know condition.Matches != nil so I believe we replace this if statement with an else if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sweet. updated.

@fchikwekwe fchikwekwe merged commit 809cd9f into main Jan 31, 2023
@fchikwekwe fchikwekwe deleted the faith/rules-convert-datatypes branch January 31, 2023 19:49
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.

3 participants