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

Adds continous metric to the deploytime #654

Merged

Conversation

mpryc
Copy link
Collaborator

@mpryc mpryc commented Sep 20, 2022

Change that adds to the deploytime continous time series for the active deployments.

Deployment with a timestamp creates an event, which may be in the past. Having just an event we are unable to make calculations based on the currently active deployments, because one may be gone from the cluster.

Adding deployment_active metric to already existing deploy_timestamp will allow to better visualize Lead Time for Change, where the "dots" are connected to each other, plus in the future we may be able to answer additional questions such as how many deployed apps are still working.

On top of that this change ensures one metric is presented only one time in the series rather then multiple times. The metrics were gathered several times multiplied by the number of found unique deployments.

Fixes #657

Signed-off-by: Michal Pryc mpryc@redhat.com

@redhat-cop/mdt

@KevinMGranger
Copy link
Collaborator

We're inheriting the mistake the previous code made-- I'll have a proposed patch to clean that up in a second.

@KevinMGranger
Copy link
Collaborator

Thoughts? KevinMGranger@1dec37a

Compare: https://gist.github.com/KevinMGranger/78bf9c10becd8d7ad9b5fe267626a68c

Not only are the metric readings now side-by-side, the statistics about python garbage collection differ by an order of magnitude. Do you accept these changes?

Change that adds to the deploytime continous time series for the active
deployments.

Deployment with a timestamp creates an event, which may be in the past.
Having just an event we are unable to make calculations based on the
currently active deployments, because one may be gone from the cluster.

Adding deployment_active metric to already existing deploy_timestamp
will allow to better visualize Lead Time for Change, where the
"dots" are connected to each other, plus in the future we may be
able to answer additional questions such as how many deployed apps
are still working.

On top of that this change ensures one metric is presented
only one time in the series rather then multiple times. The metrics
were gathered several times multiplied by the number of found
unique deployments.

Signed-off-by: Michal Pryc <mpryc@redhat.com>

Clean up deploytime

- Simplifies reporting deploy_timestamp and deployment_active
- Simplifies DeployTimeMetric creation (datetime handling)

Signed-off-by: Kevin M Granger <kgranger@redhat.com>
Co-authored-by: Kevin M Granger <kgranger@redhat.com>
@weshayutin
Copy link
Contributor

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2022
@weshayutin
Copy link
Contributor

not required for MVR.. let's so a follow up 2.0.1

@mpryc mpryc force-pushed the deployment_active_deployments branch from 02855f7 to 1dec37a Compare September 21, 2022 14:35
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2022
@KevinMGranger KevinMGranger force-pushed the deployment_active_deployments branch from 1dec37a to 02855f7 Compare September 21, 2022 15:30
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2022
@mpryc
Copy link
Collaborator Author

mpryc commented Sep 21, 2022

/retest

1 similar comment
@KevinMGranger
Copy link
Collaborator

/retest

@KevinMGranger
Copy link
Collaborator

Looks like a prow / AWS error, right?

@mpryc
Copy link
Collaborator Author

mpryc commented Sep 22, 2022

@KevinMGranger yeah

@mpryc
Copy link
Collaborator Author

mpryc commented Sep 22, 2022

/test 4.9-e2e-openshift

@openshift-ci
Copy link

openshift-ci bot commented Sep 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KevinMGranger

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2022
@KevinMGranger KevinMGranger merged commit 4cd5e30 into dora-metrics:master Sep 26, 2022
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. dco-signoff: yes do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploytime exporter reports too many same rows
4 participants