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

[faraday] Add hostname to faraday annotation #387

Closed

Conversation

tinchi
Copy link

@tinchi tinchi commented Mar 29, 2018

Faraday integration is not very informative right now:
screen shot 2018-03-29 at 5 29 07 pm
So i added hostname to the resource name

@delner delner added integrations Involves tracing integrations community Was opened by a community member labels Mar 29, 2018
@delner delner self-requested a review March 29, 2018 15:15
@delner
Copy link
Contributor

delner commented Mar 29, 2018

Thanks for the contribution @tinchi! I think this could be a good change, since you're right... GET isn't very useful in itself. The technical implementation details seem fine. I also think it might be worth considering how this interplays with the UI and the intent there.

The resource field typically represents a group of traces, each of which you might consider an instance of the same "thing". So what's the right granularity here for a Faraday trace? GET seems too broad. Adding the host seems to be better. Should it also include the path or query string too?

Without trying to expand the scope on this PR too much, I'm going to think on this a bit, so we know how this fits into making Faraday better.

@tinchi
Copy link
Author

tinchi commented Apr 2, 2018

@delner by using only hostname i would get a better overview of how particular host behaves in terms of latency and error rate. because right now i only see only latency and error rate by request type:
screen shot 2018-04-02 at 11 07 50 am
ideally i would want it to look like this:
screen shot 2018-04-02 at 11 18 41 am
but don't think its possible to implement

@delner
Copy link
Contributor

delner commented Apr 2, 2018

@tinchi Is that bottom screenshot something you photoshopped? (For sake of example?)

@tinchi
Copy link
Author

tinchi commented Apr 2, 2018

@delner yeah just few tweaks in the chrome's dev console

@delner delner added this to the 0.13.0 milestone Apr 5, 2018
@delner delner changed the base branch from master to 0.13-dev April 13, 2018 17:31
@delner
Copy link
Contributor

delner commented May 2, 2018

@tinchi Could we rebase this on 0.13-dev? Looks like there's a merge conflict since we migrated the Faraday minitests to RSpec.

@tinchi
Copy link
Author

tinchi commented May 7, 2018

@delner gonna do it shortly

@delner delner force-pushed the 0.13-dev branch 2 times, most recently from f87c11c to 0b5daa3 Compare May 8, 2018 18:38
@delner
Copy link
Contributor

delner commented Jun 4, 2018

This seems related to #277 since they are both concerned with assigning better resource names to traces of HTTP requests. We'll likely want to make the behavior consistent across each of these HTTP clients, so we should probably address what that looks like there before we move ahead with this change. This behavior is still currently up for discussion.

@delner delner removed this from the 0.13.0 milestone Jun 4, 2018
@pawelchcki pawelchcki changed the base branch from 0.13-dev to 0.14-dev June 21, 2018 10:46
@tinchi tinchi closed this Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants