Migrate from OpenCensus library #1909
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @tiffanny29631, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes a significant migration by replacing the existing OpenCensus metrics implementation with OpenTelemetry. The core purpose is to standardize on OpenTelemetry for observability, ensuring that the system continues to collect and export metrics consistently to various destinations. The changes involve updating core metric definitions, initialization routines, and exporter configurations across multiple components, along with corresponding adjustments to documentation and test utilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the project from OpenCensus to OpenTelemetry for metrics. The changes are comprehensive, covering library dependencies, metric instrumentation, recording patterns, and exporter configurations across the codebase. The test suite has also been updated to validate the new OpenTelemetry metrics. My review focuses on improving the robustness of the new implementation, particularly around shutdown contexts and resource attribute configuration.
cmd/hydration-controller/main.go
Outdated
|
|
||
| defer func() { | ||
| if err := oce.Stop(); err != nil { | ||
| if err := oce.Shutdown(context.Background()); err != nil { |
There was a problem hiding this comment.
Using context.Background() for shutdown operations can cause the process to hang if the shutdown does not complete. It is recommended to use a context with a timeout to ensure a graceful but bounded shutdown. For example:
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := oce.Shutdown(shutdownCtx); err != nil {
// ...
}This would require importing the time package.
cmd/reconciler-manager/main.go
Outdated
|
|
||
| defer func() { | ||
| if err := oce.Stop(); err != nil { | ||
| if err := oce.Shutdown(context.Background()); err != nil { |
There was a problem hiding this comment.
Using context.Background() for shutdown operations can cause the process to hang if the shutdown does not complete. It is recommended to use a context with a timeout to ensure a graceful but bounded shutdown. For example:
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := oce.Shutdown(shutdownCtx); err != nil {
// ...
}This would require importing the time package.
cmd/reconciler-manager/main.go
Outdated
| setupLog.Error(err, "problem running manager") | ||
| // os.Exit(1) does not run deferred functions so explicitly stopping the OC Agent exporter. | ||
| if err := oce.Stop(); err != nil { | ||
| if err := oce.Shutdown(context.Background()); err != nil { |
There was a problem hiding this comment.
Using context.Background() for shutdown operations can cause the process to hang if the shutdown does not complete. It is recommended to use a context with a timeout to ensure a graceful but bounded shutdown. For example:
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := oce.Shutdown(shutdownCtx); err != nil {
// ...
}This would require importing the time package.
cmd/reconciler/main.go
Outdated
|
|
||
| defer func() { | ||
| if err := oce.Stop(); err != nil { | ||
| if err := oce.Shutdown(context.Background()); err != nil { |
There was a problem hiding this comment.
Using context.Background() for shutdown operations can cause the process to hang if the shutdown does not complete. It is recommended to use a context with a timeout to ensure a graceful but bounded shutdown. For example:
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := oce.Shutdown(shutdownCtx); err != nil {
// ...
}This would require importing the time package.
| if APICallDuration == nil { | ||
| klog.Errorf("APICallDuration metric is not initialized!") | ||
| return | ||
| } |
There was a problem hiding this comment.
Checking for nil here prevents a panic, but it also means metrics will be silently dropped if InitializeOTelMetrics is not called. This could hide an initialization problem. Consider panicking here to fail fast during development and testing if a metric is used without being initialized. This would enforce that InitializeOTelMetrics is always called before any metric is recorded.
|
@tiffanny29631 will you be responding/incorporating the feedback from #1881? |
tiffanny29631
force-pushed
the
oc-migration
branch
3 times, most recently
from
25f0018 to
44c34df
Compare
Working on it now |
Change is meant to be transparent to user. Using OTel SDK for metric composing in: kmetrics, core metrics, resource group metrics; Using otlp receiver in otel-agent and otel-collector; Configured deployment for new ports and component; Refactor metric composing and recording; Metric prefix remain the same to minimize breaking change. Tests updated.
tiffanny29631
force-pushed
the
oc-migration
branch
from
44c34df to
7598e6d
Compare
|
@tiffanny29631: The following test failed, say
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. I understand the commands that are listed here. |
2 participants
Earlier discussions #1881
The migration replaces OpenCensus libraries with OpenTelemetry SDK while preserving:
Key Changes
1. Library Dependencies
Before (OpenCensus):
After (OpenTelemetry):
2. Metric Instrument Types
stats.Int64metric.Int64Counterstats.Int64metric.Int64Gaugestats.Float64metric.Float64Histogram3. Recording Patterns
Before (OpenCensus):
After (OpenTelemetry):
4. Tag/Attribute System
Before (OpenCensus):
After (OpenTelemetry):
5. Exporter Configuration
Before (OpenCensus):
After (OpenTelemetry):