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: Allow custom prometheus info metric #83

Merged
merged 7 commits into from
Mar 14, 2023

Conversation

rafvasq
Copy link
Member

@rafvasq rafvasq commented Feb 8, 2023

Motivation

Allow the generation of an "info" metric by way of environment variables captured as Gauge metric set up during container startup

Modifications

  • Added environment variable MMESH_CUSTOM_ENV_VAR in ModelMeshEnvVars
  • Added map infoMetricParams and logic to parse and pass the info to prometheus in ModelMesh
  • Added a Gauge using the parsed label names and values in Metrics
  • Added sample variables to pom.xml for testing and related test in ModelMeshMetricsTest

Result

  • Support to pass information by via environment variables parsed as <metricname>[;label1=envVarWithValueforLabel1,label2=envVarWithValueforLabel2,...,labelN=envVarWithValueforLabelN,] if provided.

Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
@rafvasq rafvasq marked this pull request as ready for review February 22, 2023 22:21
Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
@rafvasq rafvasq requested a review from njhill March 2, 2023 17:08
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

@rafvasq it doesn't look like you addressed my earlier comment about resolving env vars.

src/main/java/com/ibm/watson/modelmesh/ModelMesh.java Outdated Show resolved Hide resolved
src/main/java/com/ibm/watson/modelmesh/Metrics.java Outdated Show resolved Hide resolved
src/main/java/com/ibm/watson/modelmesh/Metrics.java Outdated Show resolved Hide resolved
Comment on lines 248 to 249
String[] labelNames = infoMetricParams.keySet().toArray(new String[0]);
String[] labelValues = infoMetricParams.values().toArray(new String[0]);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a great idea to rely on the fact that the passed in hashmap is ordered. For a regular hashmap there's no guarantee these names/values will end up in the same order. Better to create two empty arrays of the correct size prior to the loop above and just populate the names/values in the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was using a linkedhashmap here to preserve the order, I made it a bit more obvious now. Interested to know what you think or if the array creation method would still be better.

Copy link
Member

Choose a reason for hiding this comment

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

Yep I did see the LinkedHashMap but I still think it's cleaner not require it as the type. You could do:

String[] labelNames = infoMetricParams.keySet().toArray(String[]::new);
String[] labelValues = Stream.of(labelNames).map(infoMetricParams::get).toArray(String[]::new);

Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
@rafvasq
Copy link
Member Author

rafvasq commented Mar 6, 2023

@njhill thanks for the comments and patience, it's ready for another pass. I believe I'm now resolving the env vars as expected.

Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @rafvasq I think it's almost there

Comment on lines 248 to 249
String[] labelNames = infoMetricParams.keySet().toArray(new String[0]);
String[] labelValues = infoMetricParams.values().toArray(new String[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Yep I did see the LinkedHashMap but I still think it's cleaner not require it as the type. You could do:

String[] labelNames = infoMetricParams.keySet().toArray(String[]::new);
String[] labelValues = Stream.of(labelNames).map(infoMetricParams::get).toArray(String[]::new);

src/main/java/com/ibm/watson/modelmesh/ModelMesh.java Outdated Show resolved Hide resolved
Signed-off-by: Rafael Vasquez <raf.vasquez@ibm.com>
@rafvasq rafvasq requested a review from njhill March 13, 2023 17:08
@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhill, rafvasq

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@njhill
Copy link
Member

njhill commented Mar 14, 2023

/lgtm

@kserve-oss-bot kserve-oss-bot merged commit d1d0156 into kserve:main Mar 14, 2023
@rafvasq rafvasq deleted the mm-info-metric branch March 14, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved enhancement New feature or request lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants