Skip to content

Conversation

HoustonPutman
Copy link
Contributor

@HoustonPutman HoustonPutman commented Sep 17, 2019

Description

The startup options for the Prometheus Exporter are pretty sparse compared to what the Solr start script offers.

Solution

I've added some options that mirror what Solr offers, such as:

  • SOLR_HEAP
  • SOLR_JAVA_MEM
  • GC_TUNE

Having just the memory settings available would let us start the prometheus exporter with more than 500 Mb of heap, which right now isn't possible as the max heap is hard coded here.

Tests

I have started the metrics-exporter locally with this script, although I do not have access to a windows host to test the solr-exporter.cmd script. That code came from solr.cmd so I would imagine that it works, however I would appreciate if someone on windows could make sure that it works as expected.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I am authorized to contribute this code to the ASF and have removed any code I do not have a license to distribute.
  • I have developed this patch against the master branch.
  • I have run ant precommit and the appropriate test suite.
  • I have added tests for my changes.
  • I have added documentation for the Ref Guide (for Solr changes only).

@dsmiley
Copy link
Contributor

dsmiley commented Sep 17, 2019

I'm doubtful that we should use environment variables called SOLR_HEAP or SOLR_JAVA_MEM because because here the scope of use is not Solr (possibly a beefy well provisioned process), it's instead some light-weight side-car container. Maybe just JAVA_HEAP and JAVA_MEM?

Aside: I wish all Java/JVM based containers standardized the naming and semantics of such env variables, and perhaps be a bit generic referring to only "JAVA" or "JVM". It's confusing to remember which ones apply to Kafka vs Solr vs whatever.

@elyograg
Copy link
Contributor

@dsmiley I can see your point about env variable naming ... but the other side of that is that users using multiple programs might want to define all the variables for all that software in the same place, and give different heap values to each of them. If they all use the same variable, that would not be possible. Perhaps we could support a generic name as well as names prefixed with SOLR_, with the specific naming overriding the generic naming if both are defined.

@anshumg
Copy link
Contributor

anshumg commented Sep 18, 2019

Thanks @HoustonPutman .
I agree with @dsmiley w/ the naming. SOLR* will confuse users here.

Also, the precommit check seems to have failed, though glancing through it, seems like it wasn't anything related to your changes. I've just retriggered the action, and if fails again it will be worth making sure that it's unrelated (also where did it break).

@HoustonPutman
Copy link
Contributor Author

Thanks for the input. I can definitely change it to not use SOLR*, and understand how that could cause confusion.

@elyograg I definitely like the suggestion, but I'll keep that as a separate PR since I wouldn't want this to affect backwards compatibility. (If people were previously using the generic JAVA/JVM names for non-Solr things, assuming that Solr wouldn't read them)

@anshumg The precommit errors look entirely unrelated to my stuff, as it has issues with Java code, but I'll look into it more.

@dsmiley
Copy link
Contributor

dsmiley commented Sep 18, 2019

👍 Looks good to me.

@elyograg I don't think every process out their should disambiguate it's use of env vars. It's an impossible task resulting in verbose env vars (ex: SOLR_PROMETHEUS_JAVA_HEAP ?). Besides, nowadays production is increasingly container based rendering that pointless.

@anshumg
Copy link
Contributor

anshumg commented Sep 18, 2019

Looks good to me. Can you also add the ref guide changes here, and I'll merge this in right after.

@HoustonPutman
Copy link
Contributor Author

Awesome, thanks Anshum. Edited the ref guide and added a change log entry. Let me know if you want me to reword anything

@anshumg anshumg merged commit c7f8487 into apache:master Sep 18, 2019
@anshumg
Copy link
Contributor

anshumg commented Sep 18, 2019

Thanks @HoustonPutman . I'll also cherry pick this into 8x.

anshumg pushed a commit to anshumg/lucene-solr that referenced this pull request Sep 18, 2019
* SOLR-13773: Prometheus Exporter GC and Heap options

* Adding info to the ref-guide.
anshumg added a commit that referenced this pull request Sep 18, 2019
* SOLR-13773: Prometheus Exporter GC and Heap options

* Adding info to the ref-guide.
@HoustonPutman HoustonPutman deleted the metrics-options branch October 20, 2020 15:57
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.

4 participants