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

Add crd metrics usage information #2825

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add crd metrics usage information #2825

wants to merge 17 commits into from

Conversation

rubenvp8510
Copy link
Contributor

@rubenvp8510 rubenvp8510 commented Apr 7, 2024 •
edited

Description:

Add a new package that collect different metrics about the collectors on the cluster.

Link to tracking Issue(s):

Testing:

Documentation:

@rubenvp8510 rubenvp8510 requested a review from a team as a code owner April 7, 2024 21:16
@jaronoff97
Copy link
Contributor

@rubenvp8510 thanks for opening this up. Could you open an issue first so we could discuss how we could improve the observability about the operator?

@pavolloffay
Copy link
Member

Also changelog is missing

@rubenvp8510
Copy link
Contributor Author

@jaronoff97 Created the issue and linked here. @pavolloffay Added changelog.

pavolloffay
pavolloffay reviewed Apr 25, 2024
View reviewed changes
.chloggen/usage_metrics.yaml Show resolved Hide resolved
pkg/featuregate/featuregate.go Outdated Show resolved Hide resolved
pkg/crdmetrics/collector.go Outdated Show resolved Hide resolved
pkg/crdmetrics/collector.go Outdated Show resolved Hide resolved
pkg/crdmetrics/collector.go Outdated Show resolved Hide resolved
@rubenvp8510 rubenvp8510 requested a review from a team as a code owner May 7, 2024 05:07
@rubenvp8510 rubenvp8510 force-pushed the usage_metrics branch 8 times, most recently from 8956946 to 37e2e95 Compare May 7, 2024 05:36
@rubenvp8510
Add crd metrics usage information
37e2e95 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Merge branch 'main' into usage_metrics
2401708
@rubenvp8510
fix go.sum
2300bac 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Add mode metric
9948a1d 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Copy link
Contributor Author

rubenvp8510 commented May 15, 2024 •
edited

Hello @pavolloffay @jaronoff97 sorry for the delay on this PR

This is how the metrics looks like with the latest changes

# HELP opentelemetry_collector_exporters 
# TYPE opentelemetry_collector_exporters gauge
opentelemetry_collector_exporters{collector_name="simplest",namespace="default",otel_scope_name="crd-metrics",otel_scope_version="",type="debug"} 1
# HELP opentelemetry_collector_info 
# TYPE opentelemetry_collector_info gauge
opentelemetry_collector_info{collector_name="operator-check",namespace="default",otel_scope_name="crd-metrics",otel_scope_version="",type="deployment"} 0
opentelemetry_collector_info{collector_name="simplest",namespace="default",otel_scope_name="crd-metrics",otel_scope_version="",type="deployment"} 1
# HELP opentelemetry_collector_processors 
# TYPE opentelemetry_collector_processors gauge
opentelemetry_collector_processors{collector_name="simplest",namespace="default",otel_scope_name="crd-metrics",otel_scope_version="",type="batch"} 1
opentelemetry_collector_processors{collector_name="simplest",namespace="default",otel_scope_name="crd-metrics",otel_scope_version="",type="memory_limiter"} 1
# HELP opentelemetry_collector_receivers 
# TYPE opentelemetry_collector_receivers gauge
opentelemetry_collector_receivers{collector_name="simplest",namespace="default",otel_scope_name="crd-metrics",otel_scope_version="",type="otlp"} 1

Let me know if this is satisfactory and if you have more observations on this PR. Thank you very much for all the reviews!

At the end I use a gauge and the webhooks to detect creation/deletion/update, as Pavol suggested. It is a clean solution IMHO.

pavolloffay
pavolloffay reviewed May 15, 2024
View reviewed changes
apis/v1beta1/metrics.go Outdated Show resolved Hide resolved
config/manager/kustomization.yaml Outdated Show resolved Hide resolved
apis/v1beta1/metrics.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@rubenvp8510 rubenvp8510 force-pushed the usage_metrics branch 2 times, most recently from 3225b3a to 9e4448b Compare May 16, 2024 01:40
@rubenvp8510
Refactor CR metrics
9e4448b 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Copy link
Contributor Author

@pavolloffay All comments already addressed!

@rubenvp8510
Add annotation to avoid generate Metrics
7981eff 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
pavolloffay
pavolloffay reviewed May 16, 2024
View reviewed changes
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM. I would like to see some tests for the Metrics

pkg/constants/env.go Outdated Show resolved Hide resolved
@rubenvp8510
Add unit tests
d60cdde 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Merge branch 'main' into usage_metrics
4d7a8f1
@rubenvp8510
Copy link
Contributor Author

@pavolloffay this is ready for another review. Thank you!

@rubenvp8510
remove space
a1a5321 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
remove global provider
08e0db1 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
iblancasa
iblancasa reviewed May 23, 2024
View reviewed changes
config/manager/kustomization.yaml Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
rubenvp8510 and others added 2 commits May 23, 2024 08:02
@rubenvp8510 @iblancasa
Update main.go
b2d0684 
Co-authored-by: Israel Blancas <iblancasa@gmail.com>
@rubenvp8510
revert kusttomization.yaml
0683bd0 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
jaronoff97
jaronoff97 reviewed May 23, 2024
View reviewed changes
.chloggen/usage_metrics.yaml Outdated Show resolved Hide resolved
apis/v1beta1/collector_webhook.go
Comment on lines +193 to +200
warnings, err := c.validate(ctx, otelcol)
if err != nil {
return warnings, err
}

if c.metrics != nil {
c.metrics.update(ctx, otelcolOld, otelcol)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should do the metrics prior to returning, otherwise if anyone has any warnings in their CR we won't record the metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the return above (line 195) only happens if validation errors are returned, right? I'm assuming if errors were found, the collector is not created.

apis/v1beta1/collector_webhook.go
Comment on lines +211 to +218
warnings, err := c.validate(ctx, otelcol)
if err != nil {
return warnings, err
}

if c.metrics != nil {
c.metrics.delete(ctx, otelcol)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

apis/v1beta1/metrics.go Outdated Show resolved Hide resolved
apis/v1beta1/metrics.go Outdated Show resolved Hide resolved
apis/v1beta1/metrics.go Outdated Show resolved Hide resolved
apis/v1beta1/metrics.go Outdated Show resolved Hide resolved
@rubenvp8510
rename some constants
1fc64b9 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Add connectors metrics
06a11d5 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Merge branch 'main' into usage_metrics
f03ca42
@rubenvp8510
Update chlog
cec000f 
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm-sumo swiatekm-sumo Awaiting requested review from swiatekm-sumo

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@iblancasa iblancasa Awaiting requested review from iblancasa

@jaronoff97 jaronoff97 Awaiting requested review from jaronoff97

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metrics about how opentelemetry collector is used
5 participants
@rubenvp8510 @jaronoff97 @pavolloffay @iblancasa @swiatekm-sumo