Added custom tags for MVC metrics #572
Conversation
| public Iterable<Tag> getTags(HttpServletRequest request, HttpServletResponse response, Object handler, Throwable exception) { | ||
| // add tags only if enabled and the request is for PXF service endpoints (not actuator ones) | ||
| if (!customTagsEnabled || StringUtils.isBlank(request.getHeader("X-GP-USER"))) { | ||
| return EMPTY_TAGS; |
There was a problem hiding this comment.
why not just Tags.empty() which is probably already an object that was built once, no?
|
|
||
| @Override | ||
| public Iterable<Tag> getLongRequestTags(HttpServletRequest request, Object handler) { | ||
| return new ArrayList<>(); |
There was a problem hiding this comment.
did you try with long requests tags ? maybe there is no issue if we mark our controller as long request?
There was a problem hiding this comment.
Actually I think our requests are long requests, and we should probably mark them as such, and we should probably implement this method instead. Both read and write are considered long requests according to the documentation. All other requests (actuator, health) won't have the headers anyway, so nothing to add there.
There was a problem hiding this comment.
I don't yet understand the difference, I can't believe the only difference would be some arbitrary duration boundary, need more research there.
| @@ -51,11 +53,6 @@ | |||
| @Mock | |||
| private RequestContext mockContext; | |||
|
|
|||
| @BeforeAll | |||
| public static void init() { | |||
| System.setProperty("pxf.logdir", "/tmp"); | |||
There was a problem hiding this comment.
yay! did you want to default pxf.logdir to /tmp as part of this PR?
There was a problem hiding this comment.
only defaulted for my tests, I can rebase and remove this altogether and default to /tmp in application.properties
| /** | ||
| * Bean name of PXF's {@link TaskExecutor}. | ||
| */ | ||
| public static final String PXF_RESPONSE_STREAM_TASK_EXECUTOR = "pxfResponseStreamTaskExecutor"; | ||
| private static final Logger LOG = LoggerFactory.getLogger(PxfConfiguration.class); | ||
| private static final Tags EMPTY_TAGS = Tags.empty(); |
There was a problem hiding this comment.
denalex
changed the title
|
|
||
| private Tags addTag(String tag, String value, Tags tags, String defaultValue) { | ||
| value = StringUtils.defaultIfBlank(value, defaultValue); | ||
| if (StringUtils.isNotBlank(value)) { |
There was a problem hiding this comment.
this check is no longer necessary
There was a problem hiding this comment.
ah, yes, thank you for the catch, now I can remove the private method !
|
|
||
| @Override | ||
| public Iterable<Tag> getLongRequestTags(HttpServletRequest request, Object handler) { | ||
| return new ArrayList<>(); |
There was a problem hiding this comment.
can we return Tags.empty() here, if we are not going to use it?
PXF is now capable of applying custom tags to metrics reported by Spring MVC. These tags are:
user,segment,profile,serverand will be applied to metrics such ashttp.server.requests.However, due to a problem with Prometheus integration (micrometer-metrics/micrometer#2399), these tags have to be applied to all endpoints, so the actuator endpoints that receive calls not from Greenplum, will have these tags applied with the values
unknown.