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

fmt: Add basic span value filtering #22

Closed

Conversation

LucioFranco
Copy link
Member

This adds a new syntax for filtering based on values inside a span. The syntax can be used like so:

app[spanA{field:val}]=level

currently, it only works with strings due to how values are recorded into the slot.

@LucioFranco LucioFranco requested a review from hawkw March 6, 2019 23:01
Err(())
if let Some((ref field, ref val)) = directive.in_field_value {
let fields = span.fields();
let matcher = format!("{}={:?}", field, val);
Copy link
Member Author

Choose a reason for hiding this comment

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

prob could make this a lot better just wanted to get it working for my use case 😅

Copy link
Member

Choose a reason for hiding this comment

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

Having to allocate here is definitely quite unfortunate. This is a consequence of how span fields are currently represented (as a single string), which is primarily a performance optimization. We may want to reconsider that choice to make this use case a little more efficient.

tokio-trace-fmt/src/filter.rs Outdated Show resolved Hide resolved
fn parse_span_target(from: &str) -> Option<(Option<String>, Option<String>)> {
fn parse_span_target(
from: &str,
) -> Option<(Option<String>, Option<String>, Option<(String, String)>)> {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this needs to be a struct now...

part0
// part0

if let Some(part0) = part0 {
Copy link
Member

Choose a reason for hiding this comment

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

We probably should just bite the bullet at this point and replace the hand-written parser with a regex; this is really getting out of hand.

(i'm sorry for inflicting this code on you)

Copy link
Member Author

Choose a reason for hiding this comment

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

no problem! I have written way worse parsers :P

@hawkw
Copy link
Member

hawkw commented Mar 6, 2019

@LucioFranco also, I'd really prefer to use a syntax like

app[spanA{field=val}]=level

because the field=val more closely mirrors the way the formatter actually outputs fields. Note that we can differentiate between a field value filter and a level being set because one only occurs inside of curly braces and the other only occurs outside of them

@LucioFranco
Copy link
Member Author

@hawkw I like that syntax better but its just not possible with this current parser, so how I'd rather move forward is to do it this way then change, since it doesn't actually affect code just the env var.

@hawkw
Copy link
Member

hawkw commented Mar 8, 2019

@LucioFranco I'd really rather not introduce a syntax for the env var and then immediately break it...if you like, I can go ahead and rewrite the parser with a regex and then we can rebase this PR on top of that?

@LucioFranco
Copy link
Member Author

im gonna close this and open up a new one when #25 is merged.

@LucioFranco LucioFranco closed this Mar 8, 2019
@hawkw
Copy link
Member

hawkw commented Mar 8, 2019

@LucioFranco okay, the regex parser is on master now! thanks for your willingness to wait for that change and redo...

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.

2 participants