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

Micrometer metrics tags extension #1322

Merged
merged 13 commits into from Jul 20, 2021

Conversation

Kuvaldis
Copy link
Contributor

@Kuvaldis Kuvaldis commented Dec 4, 2020

Following Micrometer features added:

  1. Add timer metrics to requests resulting with exception. E.g. in MeteredClient. Includes exception name as tag.
  2. Add uri as default tag in MeteredClient metrics.
  3. Support adding custom tags in metered client, encoder, decoder etc.

Fixes #1301

@kdavisk6
Copy link
Member

@Kuvaldis looks like I may have merged your pull requests out of order. Can you check this one?

@kdavisk6 kdavisk6 added the feedback provided Feedback has been provided to the author label Dec 29, 2020
Kuvaldis added 3 commits May 15, 2021 11:00
# Conflicts:
#	micrometer/src/main/java/feign/micrometer/MeteredClient.java
#	micrometer/src/main/java/feign/micrometer/MeteredEncoder.java
@Kuvaldis
Copy link
Contributor Author

@kdavisk6 All done.

Copy link
Member

@kdavisk6 kdavisk6 left a comment

Choose a reason for hiding this comment

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

@Kuvaldis

This looks OK to me. I'd like @velo to weigh in also. I'll mark it ready and allow for some additional comments before we merge.

@kdavisk6 kdavisk6 added ready to merge Will be merged if no other member ask for changes and removed feedback provided Feedback has been provided to the author labels May 16, 2021
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

Overall, I think is a good improvement.

Things I would like to see
a) Not sure if make sense or no, but finally added to all try/catch blocks.
b) use Tag[] or Tags instead of List<Tag> + toArray(new Tag[] {}) combo
c) move metric naming to MetricName giving people one place for all customization needs.

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Tag;
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.Timer;
import java.io.IOException;
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice not to change import ordering... but, don't worry too much about it =)

return result;
}

protected List<Tag> extraSummaryTags(Response response, Type type) {
Copy link
Member

Choose a reason for hiding this comment

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

I assume you are using this for some internal metrics (I would be curious to know what you are going =] )

I would prefer to see code to live in the MetricName interface... that would give a single place for all your tag customization needs.

We would have a few methods, one called requestExtraTags other one responseExtraTags, another like exceptionExtraTags

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted tags resolution to a separate extendable class. However, seems like depending on where we create tags (i.e. client, encoder/decoder, method handler) we have really different parameters to build tags upon. I added defaultTags method, which might be helpful for customization in general.

@kdavisk6 kdavisk6 added feedback provided Feedback has been provided to the author and removed ready to merge Will be merged if no other member ask for changes labels May 21, 2021
@Kuvaldis
Copy link
Contributor Author

c) move metric naming to MetricName giving people one place for all customization needs.

Basically, there is String name(String suffix) should be enough, depending on suffix it can be overriden, for example. Another option is to create a separate method for each metric, but I think it is too specific and not really required.

@Kuvaldis Kuvaldis requested a review from velo May 23, 2021 13:06
@velo velo merged commit 5ecd704 into OpenFeign:master Jul 20, 2021
@r1jsheth
Copy link

r1jsheth commented Jan 5, 2023

is there any example depicting this use case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided Feedback has been provided to the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support uri and exception tags in micrometer MeteredClient
4 participants