Allow metrics to be sync #2433
Allow metrics to be sync #2433
Conversation
WalkthroughThis update introduces an environment variable, Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2433 +/- ##
===================================================
- Coverage 46.94226% 46.93851% -0.00375%
===================================================
Files 383 400 +17
Lines 29270 30655 +1385
Branches 83 83
===================================================
+ Hits 13740 14389 +649
- Misses 14069 14733 +664
- Partials 1461 1533 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- core/metrics/README.md (1 hunks)
- core/metrics/base.go (1 hunks)
Additional comments not posted (1)
core/metrics/base.go (1)
197-199: Ensure the new feature related toMETRICS_ALWAYS_SYNCis correctly implemented.if core.GetEnvBool("METRICS_ALWAYS_SYNC", false) { extraOpts = append(extraOpts, tracesdk.WithSampler(tracesdk.AlwaysSample())) }This code correctly checks the
METRICS_ALWAYS_SYNCenvironment variable and conditionally applies theAlwaysSample()sampler if the variable is set to true. This implementation aligns with the PR objectives and follows best practices for conditional feature toggling based on environment variables.
|
|
||
| The metrics endpoint is exposed on `/metrics` on port `8080` by default and is compatible with prometheus. The following options control the metrics endpoint: | ||
|
|
||
| | Enviornment Variable | Description | Default | | ||
| |------------------------|-----------------------------------------------|------------| | ||
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | ||
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | ||
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | Enviornment Variable | Description | Default | | ||
| |------------------------|-----------------------------------------------------------------------------|------------| | ||
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | ||
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | ||
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` | | ||
|
|
||
| **Note: this server failing to bind to `METRICS_PORT` will not cause the application to fail to start. The error will be logged.** | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3-3]
Fix the typographical and grammatical issues in the introduction paragraph.
- The metrics package contains standard drivers for opentracing, profiling and metrics.You can enable this in your appplication by calling `metrics.Setup()`. This will start the metric handler defined in the `METRICS_HANDLER` environment variable. The default is `null` which is a no-op handler.
+ The metrics package contains standard drivers for opentracing, profiling, and metrics. You can enable this in your application by calling `metrics.Setup()`. This will start the metric handler defined in the `METRICS_HANDLER` environment variable. The default is `null`, which is a no-op handler.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-8]
Correct the spelling mistakes and typographical errors in the OTLP and Jaeger descriptions.
- | OTLP | [OTLP Exporter](https://opentelemetry.io/docs/specs/otel/protocol/exporter/) protocol. Supported by various external providers including [New Relic](https://docs.newrelic.com/docs/more-integrations/open-source-telemetry-integrations/opentelemetry/opentelemetry-introduction/), [Signoz](https://signoz.io/blog/opentelemetry-collector-complete-guide/), [Grafana](https://grafana.com/docs/opentelemetry/collector/) and more | ✅ | ✅ | ❌ (but it can through pyroscope, by specifying the `PYROSCOPE_ENDPOINT` enviornment variable) |
- | Jaeger | [Jaeger](https://www.jaegertracing.io/docs/1.46/) Client Clibrary, will soon be deprecated in favor of OTLP exports to jaeger as per [this deprecation notice](https://www.jaegertracing.io/docs/1.46/client-libraries/) | ✅ | ✅ | ❌ (but it can through pyroscope, by specifying the `PYROSCOPE_ENDPOINT` enviornment variable) |
+ | OTLP | [OTLP Exporter](https://opentelemetry.io/docs/specs/otel/protocol/exporter/) protocol. Supported by various external providers including [New Relic](https://docs.newrelic.com/docs/more-integrations/open-source-telemetry-integrations/opentelemetry/opentelemetry-introduction/), [Signoz](https://signoz.io/blog/opentelemetry-collector-complete-guide/), [Grafana](https://grafana.com/docs/opentelemetry/collector/) and more | ✅ | ✅ | ❌ (but it can through pyroscope, by specifying the `PYROSCOPE_ENDPOINT` environment variable) |
+ | Jaeger | [Jaeger](https://www.jaegertracing.io/docs/1.46/) Client Library, will soon be deprecated in favor of OTLP exports to Jaeger as per [this deprecation notice](https://www.jaegertracing.io/docs/1.46/client-libraries/) | ✅ | ✅ | ❌ (but it can through pyroscope, by specifying the `PYROSCOPE_ENDPOINT` environment variable) |📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13-15]
Correct the spelling mistake in the OTLP section.
- We do our best to support enviornment variables specified in the [Otel Spec](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/) and have added a few of our own.
+ We do our best to support environment variables specified in the [Otel Spec](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/) and have added a few of our own.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [20-22]
Correct the spelling mistake in the Jaeger and Pyroscope sections.
- Pass in the `JAEGER_ENDPOINT` enviornment variable
- Pass in the `PYROSCOPE_ENDPOINT` environment variable
+ Pass in the `JAEGER_ENDPOINT` environment variable
+ Pass in the `PYROSCOPE_ENDPOINT` environment variable📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [41-41]
Correct the spelling mistake and improve the clarity in the Logger section.
- Currently, the entire sanguine codebase uses [ipfs go-log]("https://github.com/ipfs/go-log"). As pointed out in [#1521](https://github.com/synapsecns/sanguine/issues/1521), this is not a good long term solution since the logs are not currently appended to opentelemetry, and so new traces require telemtry.
+ Currently, the entire Sanguine codebase uses [ipfs go-log]("https://github.com/ipfs/go-log"). As pointed out in [#1521](https://github.com/synapsecns/sanguine/issues/1521), this is not a good long-term solution since the logs are not currently appended to OpenTelemetry, and so new traces require telemetry.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-43]
Correct the typographical and grammatical issues in the Logger section.
- In an attempt to fix this, we introduced the new context aware [otelzap logger](https://pkg.go.dev/github.com/uptrace/opentelemetry-go-extra/otelzap). Since this is in beta, you ned to call `handler.ExperimentalLogger`. In a future version, we will remove the `handler.ExperimentalLogger` and replace it with `handler.Logger` and deprecate [`ipfs/go-log`](https://github.com/ipfs/go-log).
+ In an attempt to fix this, we introduced the new context-aware [otelzap logger](https://pkg.go.dev/github.com/uptrace/opentelemetry-go-extra/otelzap). Since this is in beta, you need to call `handler.ExperimentalLogger`. In a future version, we will remove the `handler.ExperimentalLogger` and replace it with `handler.Logger` and deprecate [`ipfs/go-log`](https://github.com/ipfs/go-log).📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [51-51]
Correct the spelling and grammatical mistakes in the Limitations section.
- Currently, no enviornment variables are supported for the logger. This is a known limitation and will be fixed in a future release. Things like controlling the log level, sugarring, format, etc are [not currently supported](https://pkg.go.dev/go.uber.org/zap#NewProductionConfig). These will be added as the module beocmes more stable.
+ Currently, no environment variables are supported for the logger. This is a known limitation and will be fixed in a future release. Things like controlling the log level, sugaring, format, etc., are [not currently supported](https://pkg.go.dev/go.uber.org/zap#NewProductionConfig). These will be added as the module becomes more stable.| | Enviornment Variable | Description | Default | | ||
| |------------------------|-----------------------------------------------------------------------------|------------| | ||
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | ||
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | ||
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` | |
There was a problem hiding this comment.
Correct the spelling and grammatical mistakes in the Metrics Endpoint section.
- | Enviornment Variable | Description | Default |
- | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` |
- | `METRICS_PORT` | Port to serve metrics on. | `8080` |
- | `METRICS_PATH` | Path to serve metrics on | `/metrics` |
- | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` |
+ | Environment Variable | Description | Default |
+ | `METRICS_PORT_ENABLED` | Whether or not to enable the metrics endpoint. | `true` |
+ | `METRICS_PORT` | Port to serve metrics on. | `8080` |
+ | `METRICS_PATH` | Path to serve metrics on. | `/metrics` |
+ | `METRICS_ALWAYS_SYNC` | Whether or not to wait until metrics have been exported before ending a span| `false` |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.
| | Enviornment Variable | Description | Default | | |
| |------------------------|-----------------------------------------------------------------------------|------------| | |
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | |
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | |
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | |
| | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` | | |
| | Environment Variable | Description | Default | | |
| |------------------------|-----------------------------------------------------------------------------|------------| | |
| | `METRICS_PORT_ENABLED` | Whether or not to enable the metrics endpoint. | `true` | | |
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | |
| | `METRICS_PATH` | Path to serve metrics on. | `/metrics` | | |
| | `METRICS_ALWAYS_SYNC` | Whether or not to wait until metrics have been exported before ending a span| `false` | |
Description
Allow spans to be sync by setting an environment variable
Summary by CodeRabbit