Skip to content
This repository was archived by the owner on Mar 6, 2025. It is now read-only.

Conversation

@hecomp
Copy link

@hecomp hecomp commented Sep 21, 2023

Description

What does this PR do?

  • Added stream metrics.
  • Added hive example using jmx exporter to expose prometheus metrics

PR checklist for examples

Check these items before merging the PR.

  • Each example contains a README file.
  • README documents all steps and variables required to run the example. The simplest way to confirm this is to follow the instructions on a clean machine.
  • README file contains the links to the official documentation with relevant configuration and to the metrics published if that is available.
  • Used images of apps and collectors have to be pinned, "latest" should never be used.
  • Evidences with screenshots provided from the reviewer.
  • Includes a file called metrics.csv with metrics produced by the example.
    The file should have these 5 headings: Name, Description, Unit, DataType, Attributes.
    Description is not provided for all metrics, so it may be blank. Attributes are also not always provided. When there are multiple attributes on a metric reord they should be space separated.
  • Dashboard provided with example and put to the folder "dashboard" inside the example folder.
  • At least one screenshot of the proposed dashboard is included in this PR. If you're proposing substantive changes to queries or new queries then please ensure your screenshot shows relevant data.
  • All chart names in the dashboard are in Title case.
  • All query names in the dashboard are lower case letters, beginning the first query of each chart as "a" and proceeding alphabetically ... "b", "c", etcetera.

Copy link
Contributor

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

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

Overall looks great because it works with no fuss for Docker user.
One thing I think we should simplify a little is how the echo redirect thing writes a script in the Dockerfile. Easier to see what's going on there if you just have the script. Also I will add a couple of other minor remarks inline and we can get this merged tomorrow when addressed.

COPY --from=curler /jmx_prometheus_javaagent.jar /opt/hive/
COPY jmx_exporter.yaml /opt/hive/conf/

# Append script to hive-env.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

One way to clean this up would be put this as a script in the build context and just append the call to that script in the sh. Take 30 minutes max to clean that up for readability. If it takes longer we have people who specialize in that for you ;)

Copy link
Contributor

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

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

This needs a README. Just one of the simple ones that tells how to run it and what you get. We need that before we merge.

Otherwise... I reviewed by running and validating in logs.

@nslaughter nslaughter merged commit b3c181d into main Oct 20, 2023
@nslaughter nslaughter deleted the hsilva/hive_example branch October 20, 2023 17:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants