-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Make _source.enabled configurable for ElasticMeterRegistry #2363
Conversation
I rebased this on the current master to resolve conflicts. |
I think this could be useful to people during a proof-of-concept phase where they are exploring things in Kibana and setting up a new dashboard or something, but it's almost certainly not something that should ever be enabled in production. I have concern about how easy it would be for this to be mistakenly enabled in production by making it configurable. I'm not sure how to best mitigate that. |
@izeye Would you please add a |
@shakuzen @jonatan-ivanov Thanks for the feedback! I updated as @jonatan-ivanov suggested. |
...ns/micrometer-registry-elastic/src/main/java/io/micrometer/elastic/ElasticMeterRegistry.java
Outdated
Show resolved
Hide resolved
Can anybody explain me please, in which version this functionality was included? |
@kubickrock Se on the side:
So first this was released in |
@jonatan-ivanov please note that this change is not in 1.10 nor in 1.11. added reference to micrometer-registry-elastic 1.11 and it is not there. it is only in the 2.0 branch which has not been released. can this change be included in the next 1.x release (1.12) |
@davidfigueroaMLP thank you for bringing that to our attention. I don't think it was intentional it got dropped. I've re-opened #1629 and assigned it to the 1.12 backlog. |
@shakuzen would it make sense to assign it to the 1.11.x backlog in hope that it will be released sooner? |
@davidfigueroaMLP In general we don't consider enhancements for patch releases, but we could consider making an exception. That said, I don't see this as very critical. The configuration only controls the index template we create if missing. Once the index template is created in Elasticsearch, we will not overwrite an existing one. This means for an existing Elasticsearch cluster you've been using with Micrometer, you will need to update the index template on Elasticsearch outside of your application, anyway. So this would only help if publishing your metrics to a new cluster or if you delete the index template. Am I missing anything? Is there some reason this is more important for you that I don't realize? |
Is it released? |
But also warn about the costs associated as a mitigation to this mistakenly being enabled in a production environment. Closes gh-1629
This PR changes to make
_source.enabled
configurable forElasticMeterRegistry
. While working on this, I noticed that there's no_source.enabled
setting for Elasticsearch 6. So_source
field does exist by default in Elasticsearch 6. I didn't touch the code related to Elasticsearch 6 as it seems to be going to be dropped soon in #1906.Closes gh-1629