-
Notifications
You must be signed in to change notification settings - Fork 91
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(spans): Infer span.op if not defined #4056
Conversation
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.
Can we not build this into the existing Rule system or extend it? I'd prefer if we had a generic system instead of adding more specific stuff. Now it's the span.op
in the future it might be span.foo
. This is close to being a system which can write attributes based on rules.
for rule in self.rules { | ||
if rule.condition.matches(span) { | ||
return rule.value.clone(); | ||
} | ||
} | ||
"default".to_owned() |
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.
Untested:
for rule in self.rules { | |
if rule.condition.matches(span) { | |
return rule.value.clone(); | |
} | |
} | |
"default".to_owned() | |
self.rules.iter() | |
.find(|rule| rule.condition.matches(span)) | |
.cloned() | |
.unwrap_or("default".to_owned()) |
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 kind of like the for loop, even though it takes one more line.
@Dav1dde I considered it, the question is always, where do we stop? This PR is already a generalization of the initial idea of slapping some hard coded rules onto span normalization. But I agree that I could at least make the target field ( |
Fair enough.
I'd like that, but that may require a |
Add global option that allows inferring the span op, with rules like this:
ref: #3637