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

feat(Kubernetes): Add JMX exporter containers to all DataHub components #2251

Merged
merged 9 commits into from
Mar 31, 2021

Conversation

bernardino
Copy link
Contributor

Fixes #2214

Added JMX exporter container to Helm charts.
Also added JVM_OPTS and JMX_OPTS to such that it is easy to configure the JMX on the original container.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@shirshanka
Copy link
Contributor

@dexter-mh-lee : could you take a look?

Copy link
Contributor

@dexter-mh-lee dexter-mh-lee left a comment

Choose a reason for hiding this comment

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

Details looks good! Thanks a lot for adding this!!!

One comment on using library charts to share the jmx-exporter setup component.

@@ -74,6 +79,38 @@ spec:
{{- end }}
resources:
{{- toYaml .Values.resources | nindent 12 }}

{{- if .Values.exporters.jmx.enabled }}
- name: jmx-exporter
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to extract this out into a library chart? https://helm.sh/docs/topics/library_charts/ Seems like a lot of the code needs to be copy and pasted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I'll check that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution @bernardino, please let us know once you've been able to refactor this into a library chart. Looks great otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's running the build at the moment.

@dexter-mh-lee it looks waaay cleaner now! Thanks for your suggestion!

And I'm sorry for the delay.

Copy link
Contributor

@dexter-mh-lee dexter-mh-lee left a comment

Choose a reason for hiding this comment

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

LGTM!!! Thanks for making these changes!

@dexter-mh-lee
Copy link
Contributor

@bernardino There was a fix for the build issue that went in recently. The build error should go away once you merge latest master!

@bernardino
Copy link
Contributor Author

Build is now green

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for making the changes @bernardino 👍

@shirshanka shirshanka merged commit 9a94794 into datahub-project:master Mar 31, 2021
java $JAVA_OPTS $JMX_OPTS \
--jar jetty-util.jar \
Copy link
Contributor

Choose a reason for hiding this comment

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

@bernardino This change is breaking prod. I think java only takes a single jar. Has this specific script been tested locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I know what the error is, it should be

java $JAVA_OPTS $JMX_OPTS \
-jar /jetty-runner.jar \
--jar jetty-util.jar \
--jar jetty-jmx.jar \
/datahub/datahub-gms/bin/war.war

The --jar I added is for the jetty runner, it has nothing to do with the -jar, and in our custom run script in our helm charts the --jar options are after the runner.jar

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.

Expose operational metrics
3 participants