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

Use index prefix for template index pattern in ElasticMeterRegistry #2197

Merged
merged 1 commit into from Sep 17, 2020

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Jul 20, 2020

This PR changes to use index prefix for template index pattern in ElasticMeterRegistry.

Closes gh-2196

@shakuzen shakuzen merged commit 17f5210 into micrometer-metrics:1.1.x Sep 17, 2020
@izeye izeye deleted the gh-2196 branch September 17, 2020 03:51
@empirephoenix
Copy link

empirephoenix commented Sep 17, 2020

Hi, I'm not 100% sure, if this is the cause, but it seems related. The build produced from this
https://repo1.maven.org/maven2/io/micrometer/micrometer-registry-prometheus/1.1.18/
is missing the pom files?

" \"index_patterns\": [\"metrics*\"],\n" +
private static final Function<String, String> TEMPLATE_BODY_BEFORE_VERSION_7 = (indexPrefix) -> "{\"template\":\"" + indexPrefix + "*\",\"mappings\":{\"_default_\":{\"_all\":{\"enabled\":false}," + TEMPLATE_PROPERTIES + "}}}";
private static final Function<String, String> TEMPLATE_BODY_AFTER_VERSION_7 = (indexPrefix) -> "{\n" +
" \"index_patterns\": [\"" + indexPrefix + "*\"],\n" +

Choose a reason for hiding this comment

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

Actually though, I think the pattern should include config.indexDateSeparator()

Otherwise, the index is foo-2020-07-20 but the pattern is foo*, potentially matching foobar-2020-07-20 as well.

Choose a reason for hiding this comment

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

I just saw that my comment was still pending. I'll post it anyway now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micheljung Thanks! I created #2405 to try to resolve it.

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.

None yet

4 participants