add uptime metric [SLT-372] #3321
Conversation
WalkthroughThe changes in this pull request involve updates to the documentation in Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3321 +/- ##
===================================================
+ Coverage 31.92469% 33.24611% +1.32142%
===================================================
Files 238 543 +305
Lines 14553 34777 +20224
Branches 356 82 -274
===================================================
+ Hits 4646 11562 +6916
- Misses 9614 22190 +12576
- Partials 293 1025 +732
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
a96ea1d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f99b51a4.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-uptime-metric.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
core/metrics/base.go (2)
46-47: Correct the casing in the deprecation noticeThe deprecation notice for the
meterfield is a good practice for future maintenance. However, to adhere to Go conventions, please update the casing of the deprecation notice.Apply this change:
- // deprecated: will be removed in a future version + // Deprecated: will be removed in a future version meter MeterProvider🧰 Tools
🪛 GitHub Check: Lint (core)
[failure] 46-46:
deprecatedComment: useDeprecated:(note the casing) instead ofdeprecated:(gocritic)
Line range hint
1-268: Summary of changes and potential impactThe changes in this file appear to be part of a larger refactoring or enhancement of the metrics system:
- The
meterfield in thebaseHandlerstruct has been marked as deprecated, indicating a potential shift in how metrics are handled.- A new function
newStandardMetricshas been introduced, suggesting the implementation of a standardized metrics system.These changes, while relatively small, may have broader implications for the project's metrics handling. It would be beneficial to:
- Ensure that all parts of the codebase using the deprecated
meterfield are updated accordingly.- Thoroughly test the new metrics system to verify its correctness and performance.
- Update any relevant documentation to reflect these changes in the metrics handling approach.
Consider creating a migration plan for transitioning away from the deprecated
meterfield and adopting the new standardized metrics system across the project.🧰 Tools
🪛 GitHub Check: Lint (core)
[failure] 46-46:
deprecatedComment: useDeprecated:(note the casing) instead ofdeprecated:(gocritic)core/metrics/README.md (4)
Line range hint
13-41: LGTM! Comprehensive OTLP documentation with room for improvement.The OTLP section provides clear explanations of environment variables and introduces additional variables for multiple exporter backends. This is valuable information for users configuring the metrics package.
Consider addressing the TODO comment to fully document these options in a future update.
🧰 Tools
🪛 LanguageTool
[style] ~64-~64: The words ‘explanation’ and ‘explains’ are quite similar. Consider replacing ‘explains’ with a different word.
Context: ...s come with a# HELPexplanation that explains them, for example: ```promql # HELP pr...(VERB_NOUN_SENT_LEVEL_REP)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nguine/issues/1521), this is not a good long term solution since the logs are not current...(EN_COMPOUND_ADJECTIVE_INTERNAL)
Line range hint
54-70: LGTM! Clear explanation of the metrics endpoint with helpful examples.This section provides comprehensive information about the metrics endpoint, including configuration options and an example of a metric with HELP explanation. The note about server binding behavior is particularly important for users to understand.
Consider adding a brief explanation of what each part of the example metric represents (e.g., what
otel_scope_nameandotel_scope_versionmean) to make it even more informative for users.🧰 Tools
🪛 LanguageTool
[style] ~64-~64: The words ‘explanation’ and ‘explains’ are quite similar. Consider replacing ‘explains’ with a different word.
Context: ...s come with a# HELPexplanation that explains them, for example: ```promql # HELP pr...(VERB_NOUN_SENT_LEVEL_REP)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nguine/issues/1521), this is not a good long term solution since the logs are not current...(EN_COMPOUND_ADJECTIVE_INTERNAL)
Line range hint
72-91: LGTM! Comprehensive overview of the logging situation and future plans.This section provides valuable information about the current and future logging solutions, including limitations and known issues. The advice on using the logger with context is crucial for correct usage.
Consider adding a brief timeline or roadmap for the planned logging changes to help users prepare for the transition from
ipfs/go-logto the newotelzap logger.
Line range hint
1-91: Great job on the comprehensive README! A few minor suggestions for improvement.The README provides excellent coverage of the metrics package, with clear instructions and explanations for various components. To further enhance its quality:
Fix typos:
- Line 1: "appplication" should be "application"
- Line 37: "enviornment" should be "environment"
- Line 50: "enviornment" should be "environment"
- Line 89: "beocmes" should be "becomes"
Consider rephrasing the following for clarity:
- Line 64-65: "Most metrics come with a
# HELPexplanation that explains them" - The words "explanation" and "explains" are repetitive. Consider rewording to "Most metrics include a# HELPcomment that provides an explanation".Consistency: Ensure consistent capitalization of "environment" throughout the document.
These minor improvements will enhance the overall readability and professionalism of the README.
🧰 Tools
🪛 LanguageTool
[style] ~64-~64: The words ‘explanation’ and ‘explains’ are quite similar. Consider replacing ‘explains’ with a different word.
Context: ...s come with a# HELPexplanation that explains them, for example: ```promql # HELP pr...(VERB_NOUN_SENT_LEVEL_REP)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nguine/issues/1521), this is not a good long term solution since the logs are not current...(EN_COMPOUND_ADJECTIVE_INTERNAL)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- core/metrics/README.md (2 hunks)
- core/metrics/base.go (3 hunks)
- core/metrics/standard.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
core/metrics/README.md
[style] ~64-~64: The words ‘explanation’ and ‘explains’ are quite similar. Consider replacing ‘explains’ with a different word.
Context: ...s come with a# HELPexplanation that explains them, for example: ```promql # HELP pr...(VERB_NOUN_SENT_LEVEL_REP)
🪛 GitHub Check: Lint (core)
core/metrics/base.go
[failure] 46-46:
deprecatedComment: useDeprecated:(note the casing) instead ofdeprecated:(gocritic)
🔇 Additional comments (3)
core/metrics/base.go (1)
81-82: Provide details on the newnewStandardMetricsfunctionA new function
newStandardMetrics(ctx, b)has been added to theStartmethod. To ensure a comprehensive review:
- Could you provide more information about the purpose and functionality of this new function?
- How does it integrate with the existing metrics system?
- Are there any potential side effects or performance implications we should be aware of?
To help understand the usage and impact of this new function, please run the following script:
core/metrics/README.md (2)
Line range hint
1-11: LGTM! Clear introduction and informative table.The introduction and metrics handler table provide a comprehensive overview of the package. The deprecation notice for the Jaeger client library is particularly important for users to be aware of.
🧰 Tools
🪛 LanguageTool
[style] ~64-~64: The words ‘explanation’ and ‘explains’ are quite similar. Consider replacing ‘explains’ with a different word.
Context: ...s come with a# HELPexplanation that explains them, for example: ```promql # HELP pr...(VERB_NOUN_SENT_LEVEL_REP)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nguine/issues/1521), this is not a good long term solution since the logs are not current...(EN_COMPOUND_ADJECTIVE_INTERNAL)
Line range hint
43-52: LGTM! Concise and informative sections on Jaeger, Pyroscope, and Rookout.These sections provide the necessary information for configuring each tool, with appropriate detail levels for each. The additional configuration details for Rookout are particularly helpful.
🧰 Tools
🪛 LanguageTool
[style] ~64-~64: The words ‘explanation’ and ‘explains’ are quite similar. Consider replacing ‘explains’ with a different word.
Context: ...s come with a# HELPexplanation that explains them, for example: ```promql # HELP pr...(VERB_NOUN_SENT_LEVEL_REP)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nguine/issues/1521), this is not a good long term solution since the logs are not current...(EN_COMPOUND_ADJECTIVE_INTERNAL)
| if str.uptimeGauge, err = str.meter.Float64ObservableGauge(processUptimeSecondsMetric, metric.WithDescription("The uptime of the process in seconds"), metric.WithUnit("seconds")); err != nil { | ||
| handler.ExperimentalLogger().Errorf(ctx, "failed to create %s gauge: %v", processUptimeSecondsMetric, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialization errors are not propagated to the caller
If the creation of uptimeGauge or the registration of the callback fails, the function logs the error but does not return it to the caller. This could result in silent failures where metrics are not properly initialized, and the caller remains unaware of the issue.
Consider modifying newStandardMetrics to return an error if critical initialization steps fail, allowing the caller to handle the failure appropriately.
Suggested code change:
-func newStandardMetrics(ctx context.Context, handler Handler) *standardMetrics {
+func newStandardMetrics(ctx context.Context, handler Handler) (*standardMetrics, error) {
// existing code
if str.uptimeGauge, err = str.meter.Float64ObservableGauge(processUptimeSecondsMetric, metric.WithDescription("The uptime of the process in seconds"), metric.WithUnit("seconds")); err != nil {
- handler.ExperimentalLogger().Errorf(ctx, "failed to create %s gauge: %v", processUptimeSecondsMetric, err)
+ return nil, fmt.Errorf("failed to create %s gauge: %v", processUptimeSecondsMetric, err)
}
// Register callback
if _, err = str.meter.RegisterCallback(str.uptimeCallback, str.uptimeGauge); err != nil {
- handler.ExperimentalLogger().Warnf(ctx, "failed to register callback: %v", err)
+ return nil, fmt.Errorf("failed to register callback: %v", err)
}
+ return str, nil
}Also applies to: 32-34
| func newStandardMetrics(ctx context.Context, handler Handler) { | ||
| str := standardMetrics{ | ||
| metrics: handler, | ||
| meter: handler.Meter("standard_metrics"), | ||
| startTime: time.Now(), | ||
| } | ||
|
|
||
| var err error | ||
| if str.uptimeGauge, err = str.meter.Float64ObservableGauge(processUptimeSecondsMetric, metric.WithDescription("The uptime of the process in seconds"), metric.WithUnit("seconds")); err != nil { | ||
| handler.ExperimentalLogger().Errorf(ctx, "failed to create %s gauge: %v", processUptimeSecondsMetric, err) | ||
| } | ||
|
|
||
| // Register callback | ||
| if _, err = str.meter.RegisterCallback(str.uptimeCallback, str.uptimeGauge); err != nil { | ||
| handler.ExperimentalLogger().Warnf(ctx, "failed to register callback: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Instance of standardMetrics is not stored or returned
The newStandardMetrics function initializes a standardMetrics instance str, but it neither returns this instance nor stores it externally. As str is local to the function, it may be garbage collected after the function exits, potentially causing the uptimeCallback to malfunction due to accessing invalid or freed memory.
Consider modifying newStandardMetrics to return the *standardMetrics instance and ensure it is stored appropriately to maintain its lifecycle.
Suggested code change:
-func newStandardMetrics(ctx context.Context, handler Handler) {
+func newStandardMetrics(ctx context.Context, handler Handler) *standardMetrics {
- str := standardMetrics{
+ str := &standardMetrics{
metrics: handler,
meter: handler.Meter("standard_metrics"),
startTime: time.Now(),
}
// existing code
+ return str
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func newStandardMetrics(ctx context.Context, handler Handler) { | |
| str := standardMetrics{ | |
| metrics: handler, | |
| meter: handler.Meter("standard_metrics"), | |
| startTime: time.Now(), | |
| } | |
| var err error | |
| if str.uptimeGauge, err = str.meter.Float64ObservableGauge(processUptimeSecondsMetric, metric.WithDescription("The uptime of the process in seconds"), metric.WithUnit("seconds")); err != nil { | |
| handler.ExperimentalLogger().Errorf(ctx, "failed to create %s gauge: %v", processUptimeSecondsMetric, err) | |
| } | |
| // Register callback | |
| if _, err = str.meter.RegisterCallback(str.uptimeCallback, str.uptimeGauge); err != nil { | |
| handler.ExperimentalLogger().Warnf(ctx, "failed to register callback: %v", err) | |
| } | |
| } | |
| func newStandardMetrics(ctx context.Context, handler Handler) *standardMetrics { | |
| str := &standardMetrics{ | |
| metrics: handler, | |
| meter: handler.Meter("standard_metrics"), | |
| startTime: time.Now(), | |
| } | |
| var err error | |
| if str.uptimeGauge, err = str.meter.Float64ObservableGauge(processUptimeSecondsMetric, metric.WithDescription("The uptime of the process in seconds"), metric.WithUnit("seconds")); err != nil { | |
| handler.ExperimentalLogger().Errorf(ctx, "failed to create %s gauge: %v", processUptimeSecondsMetric, err) | |
| } | |
| // Register callback | |
| if _, err = str.meter.RegisterCallback(str.uptimeCallback, str.uptimeGauge); err != nil { | |
| handler.ExperimentalLogger().Warnf(ctx, "failed to register callback: %v", err) | |
| } | |
| return str | |
| } |
Description
Introduces an uptime metric
Summary by CodeRabbit
Documentation
METRICS_HANDLERand OTLP exporters.New Features
Refactor
meterfield in thebaseHandlerstruct.