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

chore: moving to otel from opencensus #3011

Merged
merged 30 commits into from
Jan 18, 2024

Conversation

JaydipGabani
Copy link
Contributor

@JaydipGabani JaydipGabani commented Sep 20, 2023

What this PR does / why we need it:
As opencensus is EOL, moving to opentelemetry for metrics.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #2465

Special notes for your reviewer:

  • Here is the gist of metrics export using opentelemetry exporter.
  • Here is the diff of the metrics export with opencensus/prometheus vs otel/prometheus

@JaydipGabani JaydipGabani force-pushed the opent branch 2 times, most recently from 6c39ee4 to 5033da0 Compare October 12, 2023 18:42
@JaydipGabani JaydipGabani changed the title [WIP] chore: moving to otel from opencensus chore: moving to otel from opencensus Oct 18, 2023
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out!

I left some comments on the first registry, but wanted to avoid repeating comments.

With proper encapsulation and separation-of-concerns, I think we can get this PR to the point where there are no changes outside of stats_reporter.go/tests, pkg/metrics.

pkg/audit/manager.go Outdated Show resolved Hide resolved
pkg/audit/manager.go Outdated Show resolved Hide resolved
pkg/audit/stats_reporter.go Outdated Show resolved Hide resolved
pkg/audit/stats_reporter.go Outdated Show resolved Hide resolved
pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved
pkg/metrics/exporters/stackdriver/stackdriver.go Outdated Show resolved Hide resolved
pkg/metrics/exporters/stackdriver/stackdriver.go Outdated Show resolved Hide resolved
pkg/metrics/exporters/view/view.go Outdated Show resolved Hide resolved
@JaydipGabani JaydipGabani requested a review from a team as a code owner October 24, 2023 17:28
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Attention: 87 lines in your changes are missing coverage. Please review.

Comparison is base (d2b6fcd) 53.78% compared to head (b5c3c1a) 54.44%.
Report is 1 commits behind head on master.

Files Patch % Lines
pkg/syncutil/stats_reporter.go 47.05% 27 Missing ⚠️
pkg/webhook/stats_reporter.go 78.57% 8 Missing and 4 partials ⚠️
...etrics/exporters/prometheus/prometheus_exporter.go 35.71% 9 Missing ⚠️
pkg/audit/stats_reporter.go 89.33% 6 Missing and 2 partials ⚠️
pkg/controller/mutators/stats_reporter.go 87.87% 4 Missing and 4 partials ⚠️
pkg/metrics/exporters/common/common.go 77.41% 6 Missing and 1 partial ⚠️
pkg/watch/stats_reporter.go 86.04% 4 Missing and 2 partials ⚠️
...kg/controller/constrainttemplate/stats_reporter.go 90.00% 3 Missing and 2 partials ⚠️
pkg/controller/constraint/stats_reporter.go 87.50% 2 Missing and 1 partial ⚠️
pkg/controller/expansion/stats_reporter.go 96.00% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3011      +/-   ##
==========================================
+ Coverage   53.78%   54.44%   +0.65%     
==========================================
  Files         136      134       -2     
  Lines       12195    12329     +134     
==========================================
+ Hits         6559     6712     +153     
+ Misses       5135     5122      -13     
+ Partials      501      495       -6     
Flag Coverage Δ
unittests 54.44% <80.75%> (+0.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jaydip Gabani <[email protected]>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Getting closer!

90% of comments are in the same general areas:

  • files adjacent to stats_reporter.go and stats_reporter_test.go should not have deltas (I think there was one potential exception to this)

  • All metrics state for callbacks should be maintained by reporter structs

  • All callbacks should be named like observeX

  • reportX functions should be maintained, with the same function signature (necessary for the zero-delta bullet point). In the case where callbacks are involved, this means updating reporter to have internal state and handling concurrency (locking/atomic variables).

  • Callbacks should be handled via constructing the reporter (better separation of concerns and necessary for zero delta)

pkg/audit/manager.go Outdated Show resolved Hide resolved
pkg/audit/manager.go Outdated Show resolved Hide resolved
pkg/audit/stats_reporter.go Show resolved Hide resolved
pkg/controller/sync/sync_controller.go Outdated Show resolved Hide resolved
pkg/controller/sync/sync_controller.go Outdated Show resolved Hide resolved
pkg/syncutil/stats_reporter.go Outdated Show resolved Hide resolved
pkg/watch/manager.go Outdated Show resolved Hide resolved
pkg/watch/registrar.go Outdated Show resolved Hide resolved
pkg/watch/stats_reporter.go Show resolved Hide resolved
pkg/controller/mutators/stats_reporter.go Outdated Show resolved Hide resolved
@JaydipGabani
Copy link
Contributor Author

@maxsmythe Thanks for providing detailed feedback. I think I addressed all of your feedback. PTAL!

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Getting close! Biggest themes are:

  • observe functions need to acquire a read mutex
  • I think we can consolidate a lot of test helper code

Some other, less global comments. Once this is done, we can help make sure stackdriver doesn't have a diff.

pkg/audit/stats_reporter.go Show resolved Hide resolved
pkg/audit/stats_reporter.go Show resolved Hide resolved
pkg/controller/constraint/stats_reporter.go Outdated Show resolved Hide resolved
pkg/controller/constraint/stats_reporter_test.go Outdated Show resolved Hide resolved
pkg/controller/expansion/stats_reporter.go Show resolved Hide resolved
pkg/controller/expansion/stats_reporter.go Show resolved Hide resolved
pkg/watch/manager.go Outdated Show resolved Hide resolved
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

LGTM pending @acpana's approval for stackdriver

@JaydipGabani JaydipGabani requested review from acpana and ritazh January 16, 2024 21:20
Signed-off-by: Jaydip Gabani <[email protected]>
Comment on lines +24 to +25
// AddReader adds a reader to the options and updates the MeterProvider if the required conditions are met.
func AddReader(opt metric.Option) {
Copy link
Contributor

@acpana acpana Jan 16, 2024

Choose a reason for hiding this comment

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

api nits: if we keep this structure, it may be useful to at least spell out the "required conditions". Alternatively splitting this into a AddReader and SetMetricsProvider might offer a cleaner/ more concise api

Signed-off-by: Jaydip Gabani <[email protected]>
@JaydipGabani JaydipGabani requested a review from acpana January 17, 2024 21:29
Copy link
Contributor

@acpana acpana left a comment

Choose a reason for hiding this comment

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

As of b5c3c1a, I don't see a delta to metrics sent to GCM (formerly stackdriver). Thank you loads Jay for working on this 💯 🥇

@JaydipGabani JaydipGabani enabled auto-merge (squash) January 18, 2024 17:11
@JaydipGabani JaydipGabani disabled auto-merge January 18, 2024 17:14
@JaydipGabani JaydipGabani enabled auto-merge (squash) January 18, 2024 17:28
@sozercan sozercan dismissed maxsmythe’s stale review January 18, 2024 17:48

Max LGTM'ed in a comment

@JaydipGabani JaydipGabani merged commit 7692a33 into open-policy-agent:master Jan 18, 2024
17 of 18 checks passed
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

LGTM

leewoobin789 pushed a commit to softlee-io/gatekeeper that referenced this pull request Apr 1, 2024
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydipkumar Arvindbhai Gabani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for opentelemetry for metrics
6 participants