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.

Sign up for GitHub

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

Jump to bottom

Emit Cluster Feature Metrics for Cluster Operations #3631

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Emit Cluster Feature Metrics for Cluster Operations #3631

merged 2 commits into from
Jul 8, 2024

Conversation

s-fairchild
Copy link
Collaborator

@s-fairchild s-fairchild commented Jun 12, 2024 •
edited
Loading

Which issue this PR addresses:

ARO-7839

Fixes

What this PR does / why we need it:

Emit metrics for all RP operation types, creating, updating, adminupdating, and deleting.

Test plan for issue:

Covered by unit tests.

Is there any documentation that needs to be updated for this PR?

No

How do you know this will function as expected in production?

@s-fairchild s-fairchild added enhancement New feature or request chainsaw Pull requests or issues owned by Team Chainsaw labels Jun 12, 2024
@s-fairchild s-fairchild changed the title S fairchild/emit install metrics Emit cluster feature metrics at install time Jun 12, 2024
@s-fairchild s-fairchild force-pushed the s-fairchild/emit-install-metrics branch 9 times, most recently from 0290d4f to 4288b86 Compare June 20, 2024 20:03
@s-fairchild s-fairchild marked this pull request as ready for review June 20, 2024 20:19
tsatam
tsatam reviewed Jun 20, 2024
View reviewed changes
Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

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

I have some questions around the specific dimensions we're emitting - I want to make sure that each individual dimension we include on these metrics is explicitly useful to us so we're not introducing noise into our observability.

pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
kimorris27
kimorris27 requested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

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

Good work! I recommended adding unit tests for emitMetrics and left a few other smaller question/concerns.

pkg/backend/openshiftcluster.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
pkg/backend/metrics.go Outdated Show resolved Hide resolved
@s-fairchild s-fairchild force-pushed the s-fairchild/emit-install-metrics branch 2 times, most recently from 4d24062 to 27a3684 Compare June 21, 2024 21:37
hawkowl
hawkowl reviewed Jun 26, 2024
View reviewed changes
pkg/backend/metrics.go Outdated Show resolved Hide resolved
@s-fairchild s-fairchild force-pushed the s-fairchild/emit-install-metrics branch from 6048e8c to ecbdf64 Compare June 26, 2024 21:36
@s-fairchild
Emit cluster features enabled/disabled during cluster provisioning
9d1aa64 
Add workload identity or cluster service principal metrics emission
Add emit features
Emit features concerning newly installed clusters.
Add emitMetrics to all provisioning states
Add logging statements for metrics,
Allows metrics being emitted to also be used in kusto dashboards.
Constant values are used for all metric names in deminsions.
If an error is encountered while gathering metrics, that metric is omitted.
Unit tests provide 100% coverage for emitMetrics.

Rename emitMetrics to emitProvisioningMetrics, rename emitInstallMetrics to emitMetrics,
emitMetrics can be used for all provisioning states, not just creating state. Changed name to reflect this.
@s-fairchild s-fairchild force-pushed the s-fairchild/emit-install-metrics branch from ecbdf64 to 9d1aa64 Compare June 27, 2024 01:32
@kimorris27
Copy link
Contributor

/azp run ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

kimorris27
kimorris27 previously approved these changes Jun 27, 2024
View reviewed changes
@s-fairchild s-fairchild force-pushed the s-fairchild/emit-install-metrics branch from 78ff980 to a99acf9 Compare June 27, 2024 21:08
@kimorris27
Copy link
Contributor

/azp run ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@s-fairchild s-fairchild changed the title Emit cluster feature metrics at install time Emit cluster feature metrics for cluster operations Jul 3, 2024
@s-fairchild s-fairchild changed the title Emit cluster feature metrics for cluster operations Emit Cluster Feature Metrics for Cluster Operations Jul 3, 2024
cadenmarchese
cadenmarchese approved these changes Jul 8, 2024
View reviewed changes
Copy link
Collaborator

@cadenmarchese cadenmarchese left a comment

Choose a reason for hiding this comment

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

lgtm

@cadenmarchese
Copy link
Collaborator

/azp run e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cadenmarchese cadenmarchese merged commit 3db4360 into master Jul 8, 2024
21 checks passed
anshulvermapatel added a commit that referenced this pull request Jul 15, 2024
@anshulvermapatel
reverted pr #3631
cb2bf0e
@SudoBrendan SudoBrendan deleted the s-fairchild/emit-install-metrics branch July 24, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkowl hawkowl hawkowl left review comments

@tsatam tsatam tsatam left review comments

@swiencki swiencki swiencki left review comments

@cadenmarchese cadenmarchese cadenmarchese approved these changes

@kimorris27 kimorris27 kimorris27 approved these changes

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@bennerv bennerv Awaiting requested review from bennerv bennerv is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@jaitaiwan jaitaiwan Awaiting requested review from jaitaiwan jaitaiwan is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

@tiguelu tiguelu Awaiting requested review from tiguelu tiguelu is a code owner

@SudoBrendan SudoBrendan Awaiting requested review from SudoBrendan SudoBrendan is a code owner

Assignees
No one assigned
Labels
chainsaw Pull requests or issues owned by Team Chainsaw enhancement New feature or request ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@s-fairchild @kimorris27 @cadenmarchese @hawkowl @tsatam @swiencki