Skip to content

Conversation

@crawford
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 26, 2019
@crawford crawford changed the title Cluster installer metrics: gather cluster_installer series Jun 26, 2019
@crawford
Copy link
Contributor Author

/cc @smarterclayton

@squat
Copy link
Contributor

squat commented Jun 26, 2019

@crawford do you have a running cluster you tested this on? if so, could you also run:

make test/timeseries.txt
make generate

to get the new metric in the test/timeseries.txt file and also generate an updated docs/sample-metrics.md?

If you don't have one on hand, don't worry, we can do a follow up :)

@crawford
Copy link
Contributor Author

@squat my generated timeseries data is significantly different than the last. Is that a concern?

@squat
Copy link
Contributor

squat commented Jun 26, 2019

@crawford not an issue at all. It’s expected that pretty much every line should be different as timestamps and values will all change.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 26, 2019
@crawford
Copy link
Contributor Author

@squat updated. make generate didn't produce any new assets. I'm not sure how to get that doc updated.

@squat
Copy link
Contributor

squat commented Jun 26, 2019

Sorry to complicate things. The makefile has recently changed. Plz try make generate -B or make generate-in-docker -B to force and build in a container.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 2019
@crawford
Copy link
Contributor Author

@squat Okay, make generate -B did the trick. Is there an existing process for approving what gets whitelisted?

@sichvoge
Copy link

sichvoge commented Jun 27, 2019

There is an internal approval chain for that. Currently, the monitoring team working on Telemeter and I approve new metrics that people like to push back. The engineering team makes sure that the metric is ok to send (specifically taking cardinality into account) and I just want to understand what the use cases are and if that aligns with Telemeters goal.

TD;LR: you need two LGTM: 1) telemeter engineer and 2) me :)

@squat
Copy link
Contributor

squat commented Jun 27, 2019

/hold

From telemeter engineering team, this looks good to me. The new time series do not present a burden and are well within our current capacity.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2019
@squat
Copy link
Contributor

squat commented Jun 27, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 27, 2019
@sichvoge
Copy link

I am actually not really sure what the following means:

cluster_installer reports what installed the cluster, along with its version number and invoker.

What information are we sending off-cluster? Do you have an example?

@squat
Copy link
Contributor

squat commented Jun 27, 2019

Ah, that should have been captured in the file I asked @crawford to generate, test/time-series.txt. But I see now that it's not there. @crawford the metric does not appear to be in Prometheus. Is the service exposing this metric not yet being scraped?

@crawford
Copy link
Contributor Author

Hmm, let me take a closer look. I'll update the commit message with an example as well.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
@crawford
Copy link
Contributor Author

crawford commented Jul 3, 2019

@squat Okay, this should be ready for review now.

@crawford
Copy link
Contributor Author

crawford commented Jul 5, 2019

/retest

@crawford
Copy link
Contributor Author

crawford commented Jul 8, 2019

/retest

@smarterclayton
Copy link
Contributor

/approve

This metric is approved at a product level (high value in both triage and identification of unique issues)

Copy link
Contributor

@squat squat left a comment

Choose a reason for hiding this comment

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

/lgtm

Spoke with Alex about this and looks good :) I’m glad we got this in

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@crawford
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2019
crawford added 3 commits July 15, 2019 09:24
Rather than hardcoding the path to bash (which is not always in /bin),
use `env` to find it.
This series will allow us to determine whether a cluster was created
using IPI or UPI. Once Hive and CI are updated, we'll also be able to
determine if a cluster was created by CI or if it was created by Hive.

The series captures the install type, the version of the tool, and its
invoker (in addition to the standard labels):

    cluster_installer{
        type="openshift-install",
        version="unreleased-master-1209-gfd08f44181f2111",
        invoker="alex",
    } 0 1562168623759
This new sample includes the new cluster_installer series.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@crawford
Copy link
Contributor Author

Rebased onto master. @squat can you take one more look?

@squat
Copy link
Contributor

squat commented Jul 15, 2019

ahh we had another conflict

@squat
Copy link
Contributor

squat commented Jul 15, 2019

/lgtm
cmooooooooon

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, smarterclayton, squat

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [smarterclayton,squat]

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

@smarterclayton
Copy link
Contributor

smarterclayton commented Jul 15, 2019 via email

@crawford
Copy link
Contributor Author

@smarterclayton yes it does. Where did the server manifests go?

@squat
Copy link
Contributor

squat commented Jul 15, 2019

It looks like last week the telemeter-server manifests changed home to their new v2 location in github.com/observatorium/configuration. The important thing is to update the metrics.json file. This is the source of truth and will get picked up in the new v2 configuration repo (including the telemeter-server whitelist) the next time we bump.

However, it seems that the implications of this change were not completely considered. We’ll need to either revert or adjust the CI job to reflect this.

@crawford
Copy link
Contributor Author

@squat cool. Do you know what's up with those two failed tests? Did I do something wrong in the PR or should I just kick them again?

@squat
Copy link
Contributor

squat commented Jul 15, 2019

@crawford let me dig and take a looksie

@crawford
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 0bacffc into openshift:master Jul 16, 2019
@crawford crawford deleted the cluster_installer branch July 16, 2019 16:46
@crawford
Copy link
Contributor Author

crawford commented Aug 5, 2019

/cherrypick release-4.1

@openshift-cherrypick-robot

@crawford: #189 failed to apply on top of branch "release-4.1":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	Makefile
Falling back to patching base and 3-way merge...
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
Patch failed at 0001 Makefile: improve portability of shell

Details

In response to this:

/cherrypick release-4.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants