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

Get metrics even a request fails #336

Closed
dotSlashLu opened this issue Aug 8, 2021 · 5 comments · Fixed by #337
Closed

Get metrics even a request fails #336

dotSlashLu opened this issue Aug 8, 2021 · 5 comments · Fixed by #337
Labels
feature A new feature!

Comments

@dotSlashLu
Copy link

Hi, thanks for this great lib!

I'm using it to implement a network debugging tool for our client, utilizing it's great metrics interface. In this context, error is expected and I want to extract metrics from the request. Is this currently possible? The phase throwing the error can be observed from the error's Kind, so specifically I need remote_addr of this failed request.

@sagebind
Copy link
Owner

sagebind commented Aug 8, 2021

Thanks for filing an issue! It is not currently possible to get access to a Metrics handle without a Response, but I can see how that might be useful.

so specifically I need remote_addr of this failed request.

There may or may not be a remote address depending on whether we actually managed to connect to a server or not. Though, the remote address isn't part of the metrics API but a different method. This seems pretty useful to add though to the Error type, we just have to make sure that the Error struct doesn't become too bloated at the risk of performance penalty.

What if we added a method to Error like this?

impl Error {
    /// Get the remote socket address of the last-used connection involved in
    /// this error, if known.
    ///
    /// If the request that caused this error failed to connect to any server,
    /// then this will be `None`.
    pub fn remote_addr(&self) -> Option<SocketAddr>;
}

@sagebind sagebind added the feature A new feature! label Aug 8, 2021
@dotSlashLu
Copy link
Author

dotSlashLu commented Aug 8, 2021

That would be really nice in my scenario!

However, like you said, this implementation might bloat the Error type. Due to my limited experience on Error interface design, I can't give you any insightful opinion on this. But maybe it's better to just tuck this information in the error's context? For example, "Timed out connecting to {SockAddr}"?

@sagebind
Copy link
Owner

sagebind commented Aug 9, 2021

Are you just looking to have a more descriptive error message? Or do you need to actually extract the remote address for something? Because trying to scrape it from the error message or error context sounds risky from a stability perspective if the error message format changes (which isn't part of the API stability guarantee).

@sagebind
Copy link
Owner

I have a branch that adds local_addr and remote_addr methods to the Error type in a branch here: #337. I see no issue with exposing these since Error already heap-allocates in order to avoid taking up too much stack space. (You don't want to penalize the performance of the success path by having an error type that takes up a lot of stack space.)

sagebind added a commit that referenced this issue Aug 22, 2021
Add `local_addr` and `remote_addr` to `Error` which expose the local and remote network addresses of the last used connection for a request before the error occurred, if known.

Fixes #336.
@sagebind
Copy link
Owner

This is now exposed in the 1.5.0 release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants