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

[named] provides an identifier for each instrumented client #23

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

esiqveland
Copy link
Contributor

This is a suggestion for providing easily identifiable metrics for individual clients.

Lets you provide an identifier for each instrumented client.
This is very useful when looking at metrics in applications that consume
services from many other services and lets you see which service a client is using.

Adresses #22.

@esiqveland
Copy link
Contributor Author

Sorry, I wanted to keep this addition in one commit, but all your comments disappeared after I resubmitted :(

Just wanted to say I am open for more discussion as well, I fixed a lot of just style comments and such.

Edit:
Found the comments here: esiqveland@fe45e75#comments

@raskasa
Copy link
Owner

raskasa commented Nov 23, 2015

Ping!

Provides easily identifiable metrics for individual clients.

Lets you provide an identifier for each instrumented client.
This is very useful when looking at metrics in applications that consume
services from many other services and lets you see which service a client is using.

Adresses raskasa#22.
@esiqveland
Copy link
Contributor Author

Hey, I did not have time to look this over until now.

Fixed the comments and finally the whitespace xD

I like keeping work in one coherent commit, but it does not play very nice with PR comments.
Please tell if you prefer lots of small commits.

Pong!

raskasa added a commit that referenced this pull request Nov 30, 2015
[named] provides an identifier for each instrumented client
@raskasa raskasa merged commit 11757af into raskasa:master Nov 30, 2015
@raskasa
Copy link
Owner

raskasa commented Nov 30, 2015

I prefer one coherent commit as well. Thanks!

@raskasa
Copy link
Owner

raskasa commented Nov 30, 2015

I've thought a little more. I don't think we should have a name by default.

The library authors designed the library with the intention for users to have one OkHttpClient per application. My assumption is that this happens in a majority of cases. I'm thinking that the name should be null by default.

Then the metric names can look like this by default:

...
com.squareup.okhttp.OkHttpClient.cache-write-abort-count
com.squareup.okhttp.OkHttpClient.connection-pool-count
...

And like this if it has a custom name:

...
com.squareup.okhttp.OkHttpClient.custom-name.cache-write-abort-count
com.squareup.okhttp.OkHttpClient.custom-name.connection-pool-count
...

I can make this change myself in a separate PR. Either way, thoughts?

@raskasa
Copy link
Owner

raskasa commented Dec 14, 2015

Closes #22

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