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

Allow users to control level of Tracing spans #124

Merged
merged 6 commits into from
Nov 15, 2021

Conversation

hannobraun
Copy link
Contributor

Motivation

I want to see the request method and path in my log message. See #123 for more information.

Solution

This is a minimally invasive solution for #123: It allows the user to configure the level for request spans. While this is far from ideal API-wise, it's a quick fix. I don't think I know enough about how Tracing is supposed to be used to come up with something better right now (that spans have associated levels in the first place seems wrong to me, but I'm sure I'm missing something).

@hannobraun hannobraun changed the title Trace Allow users to control level of Tracing spans Aug 19, 2021
@davidbarsky
Copy link
Contributor

davidbarsky commented Aug 19, 2021

Whoops! Sorry—I've accidently closed this PR before I wrote my message. I also didn't meant to close it! Gimme a second as I write the message!

@davidbarsky davidbarsky reopened this Aug 19, 2021
@davidbarsky
Copy link
Contributor

(I'm putting on my "tracing contributor" hat on.)

Anyways! I noticed that in #123, you mentioned that you're creating the TraceLayer thusly:

TraceLayer::new_for_http()
    .on_request(trace::DefaultOnRequest::new().level(Level::INFO))
    .on_response(trace::DefaultOnResponse::new().level(Level::INFO))

It seems to me that TraceLayer::make_span_with—as documented here)—should address your use-case without needing to introduce a new, slightly different API. Am I incorrect?

@hannobraun
Copy link
Contributor Author

Oh, you're right! I was so focused on the DefaultMakeSpan, I somehow missed that you can pass your own MakeSpan implementation there. That addresses my use case.

Thank you!

@hannobraun hannobraun closed this Aug 19, 2021
@davidpdrsn davidpdrsn reopened this Aug 19, 2021
@davidpdrsn
Copy link
Member

I've opened. I still think it would be nice to address this inconsistency. Will review tomorrow.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Sorry the delay!

I think this looks except for one small thing wrt the docs.

Also wanna add a line in the changelog?

You should also merge/rebase master to fix CI.

tower-http/src/trace/on_request.rs Show resolved Hide resolved
@hannobraun
Copy link
Contributor Author

Thanks for the review and also sorry for the delay! I'm on vacation. I'll take care of this at some point during the next weeks.

@davidpdrsn
Copy link
Member

No worries :)

This doesn't make sense as-is, but I'm about to add an additional
non-`Default` field to `DefaultMakeSpan`. It will no longer be possible
to derive `Default` once that happens.
This is a minimally invasive fix for tower-rs#123. It's certainly not ideal,
API-wise, but at least it allows users to add request information to
their logs without having to set the log level to DEBUG.

Close tower-rs#123
Those no longer need to be speicified explicitly in recent versions of
rustdoc.
@hannobraun
Copy link
Contributor Author

Rebased on top of master (no changes were necessary). Added changelog entry as requested. Did not address the review comment, as I disagree with the request (see comment there).

@davidpdrsn davidpdrsn added this to the 0.1.2 milestone Nov 8, 2021
@davidpdrsn
Copy link
Member

@hannobraun Sorry for the delay on this. I think it looks good as is 😊 Do you wanna merge/rebase master? I would like to include this in 0.1.2 which I'm hoping to release next week.

@davidpdrsn davidpdrsn modified the milestones: 0.1.2, 0.2.0 Nov 13, 2021
@davidpdrsn davidpdrsn merged commit c3a1fcb into tower-rs:master Nov 15, 2021
@davidpdrsn
Copy link
Member

@hannobraun Thanks for working on this! Sorry for taking so long with reviews.

@hannobraun hannobraun deleted the trace branch November 15, 2021 12:40
@hannobraun
Copy link
Contributor Author

Looks like you got it sorted before I had a chance to look into it. Thanks!

And don't worry about the time it took. There are only so many hours in the day, after all 😁

@davidpdrsn davidpdrsn mentioned this pull request Dec 1, 2021
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