-
Notifications
You must be signed in to change notification settings - Fork 50
Migrate from OpenCensus library #1881
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
Conversation
Skipping CI for Draft Pull Request. |
[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 |
aa7bce5
to
6703484
Compare
89f4b05
to
8d2005f
Compare
7dfe3c9
to
c8772b9
Compare
9b6aeca
to
8544c2f
Compare
} | ||
|
||
func TestApply(t *testing.T) { | ||
_ = testmetrics.NewTestExporter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this line doing? Consider adding a comment explaining the purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored testexporter.go in #1909 for readability and using ResetGlobalMetrics when no exporter is needed. Comment is added to the func signature. ResetGlobalMetrics does the similar to NewTestExporter expects not returning an exporter, only initializing metrics and meter provider. The function needs to be explicitly invoked to respect the nature of how metrics are initialized in OtelSDK.
Tags: []tag.Tag{ | ||
{Key: metrics.KeyInternalErrorSource, Value: "parser"}, | ||
}, | ||
Name: "declared_resources", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a change in behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, declared_resources is part of the metric entries that are recorded in this test, the validateMetrics checks if the expectedMetrics is a subset of the entire metric storage, so it's additional coverage. But since it's irrelevant to the specific test, I removed it in #1909 for reduced confusion.
} | ||
|
||
// Global test reader to capture metrics | ||
var globalTestReader sdkmetric.Reader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the global reader necessary? The previous scaffolding seems to have been able to do this without a global
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testexporter.go is refactored in #1909 and global reader is no longer needed.
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.
8544c2f
to
ac1af18
Compare
/gemini summary |
Pull Request SummaryThis pull request migrates the project's metrics implementation from OpenCensus to OpenTelemetry SDK. The migration is comprehensive, ensuring that existing metric names, types, descriptions, recording patterns, pipeline architecture, data flow, sidecar configurations, and export destinations (Prometheus, Cloud Monitoring, Cloud Monarch) are preserved. Key changes include:
This migration ensures the project aligns with the latest observability standards and best practices provided by the OpenTelemetry ecosystem. Code ReviewOverall, this is a well-executed and comprehensive migration from OpenCensus to OpenTelemetry. The changes are consistent across the codebase, addressing all necessary aspects from dependency management to metric instrumentation, recording, exporting, and testing. The adherence to OpenTelemetry's API and best practices is evident throughout the modified files. I recommend merging this pull request. |
this is closed, but is there a follow up? Are we giving up on this project? |
The PR was opened on a wrong branch. Will reopen new one from fork and address comments from there. |
The migration replaces OpenCensus libraries with OpenTelemetry SDK while preserving:
Key Changes
1. Library Dependencies
Before (OpenCensus):
After (OpenTelemetry):
2. Metric Instrument Types
stats.Int64
metric.Int64Counter
stats.Int64
metric.Int64Gauge
stats.Float64
metric.Float64Histogram
3. Recording Patterns
Before (OpenCensus):
After (OpenTelemetry):
4. Tag/Attribute System
Before (OpenCensus):
After (OpenTelemetry):
5. Exporter Configuration
Before (OpenCensus):
After (OpenTelemetry):