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

Make _source.enabled configurable for ElasticMeterRegistry #2363

Merged
merged 2 commits into from
Mar 28, 2022

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Nov 27, 2020

This PR changes to make _source.enabled configurable for ElasticMeterRegistry. 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

@izeye
Copy link
Contributor Author

izeye commented Nov 27, 2020

I rebased this on the current master to resolve conflicts.

@shakuzen
Copy link
Member

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.

@jonatan-ivanov
Copy link
Member

@izeye Would you please add a WARN message to every publish that warns the users that source is enabled? This way people would be able to turn this on but will be warned so that they have a chance to catch this in prod.

@shakuzen shakuzen added the waiting for feedback We need additional information before we can continue label Mar 25, 2022
@izeye
Copy link
Contributor Author

izeye commented Mar 25, 2022

@shakuzen @jonatan-ivanov Thanks for the feedback!

I updated as @jonatan-ivanov suggested.

@shakuzen shakuzen removed the waiting for feedback We need additional information before we can continue label Mar 25, 2022
@jonatan-ivanov jonatan-ivanov added enhancement A general enhancement registry: elastic An ElasticSearch Registry related issue labels Mar 25, 2022
@jonatan-ivanov jonatan-ivanov added this to the 2.0.0-M4 milestone Mar 25, 2022
@shakuzen shakuzen merged commit 8bf66dc into micrometer-metrics:main Mar 28, 2022
@izeye izeye deleted the gh-1629 branch April 21, 2022 04:48
@kubickrock
Copy link

Can anybody explain me please, in which version this functionality was included?

@jonatan-ivanov
Copy link
Member

@kubickrock Se on the side:

Milestone
1.10.0-M1

So first this was released in 1.10.0-M1, if you want to know the first stable/GA release that's without the -M1 so 1.10.0.

@davidfigueroaMLP
Copy link

@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)

https://github.com/micrometer-metrics/micrometer/blob/2.0.x/implementations/micrometer-registry-elastic/src/main/java/io/micrometer/elastic/ElasticConfig.java#L191

@shakuzen
Copy link
Member

shakuzen commented Jun 7, 2023

@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.

@davidfigueroaMLP
Copy link

@shakuzen would it make sense to assign it to the 1.11.x backlog in hope that it will be released sooner?

@shakuzen
Copy link
Member

shakuzen commented Jun 9, 2023

@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?

@denistsyplakov
Copy link

Is it released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement registry: elastic An ElasticSearch Registry related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable _source.enabled Elastic mapping property
6 participants