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

Add parameter to optionally strip deploy path. #639

Merged
merged 2 commits into from Jul 31, 2021

Conversation

lapo-luchini
Copy link
Contributor

Closes #631.

Closes prometheus#631.

Signed-off-by: Lapo Luchini <lapo@lapo.it>
Copy link
Member

@fstab fstab left a comment

Choose a reason for hiding this comment

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

Thanks a lot or the PR. Here are a few minor comments:

Please update the class JavaDoc for MetricsFilter. Add the new option with its default to the example:

    <!-- strip-context-path is optional, defaults to false -->
   <init-param>
      <param-name>strip-context-path</param-name>
      <param-value>false</param-value>
   </init-param>

...and add an explanation of the strip-context-path option to the text above the example.

Additionally, please also update the Servlet Filter section in README.md.

There should be a test for the new option in MetricsFilterTest. With copy-and-paste from init() it could look something like this

    @Test
    public void testStripContextPath() throws Exception {
        String metricName = "foo";
        FilterConfig cfg = mock(FilterConfig.class);
        when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn(metricName);
        when(cfg.getInitParameter(MetricsFilter.PATH_COMPONENT_PARAM)).thenReturn("0");
        when(cfg.getInitParameter(MetricsFilter.STRIP_CONTEXT_PATH_PARAM)).thenReturn("true");
        f.init(cfg);
        assertTrue(f.stripContextPath);
        HttpServletRequest req = mock(HttpServletRequest.class);
        when(req.getRequestURI()).thenReturn("/foo/bar/baz/bang");
        when(req.getContextPath()).thenReturn("/foo/bar");
        when(req.getMethod()).thenReturn(HttpMethods.GET);
        HttpServletResponse res = mock(HttpServletResponse.class);
        FilterChain c = mock(FilterChain.class);
        f.doFilter(req, res, c);
        verify(c).doFilter(req, res);
        final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method"}, new String[]{"/baz/bang", HttpMethods.GET});
        assertNotNull(sampleValue);
        assertEquals(1, sampleValue, 0.0001);
    }

Signed-off-by: Lapo Luchini <lapo@lapo.it>
@lapo-luchini
Copy link
Contributor Author

@fstab let me know if anything more should be changed.

@fstab fstab merged commit d663d04 into prometheus:master Jul 31, 2021
@fstab
Copy link
Member

fstab commented Jul 31, 2021

Thanks a lot @lapo-luchini, and sorry that it took so long to merge this.

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.

Avoid deploy path in the MetricsFilter
2 participants