feat: logs SDK observability - otlploggrpc exporter metrics#7084
feat: logs SDK observability - otlploggrpc exporter metrics#7084yumosx wants to merge 61 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7084 +/- ##
=======================================
+ Coverage 82.9% 83.0% +0.1%
=======================================
Files 267 270 +3
Lines 24997 25201 +204
=======================================
+ Hits 20743 20942 +199
- Misses 3878 3881 +3
- Partials 376 378 +2
🚀 New features to boost your workflow:
|
|
Good suggestion, done. |
- use atomic.Int64 - use ErrorType
|
Could you add to the CHANGELOG |
sure. |
|
this ci/codecov seems to need to be run again.
|
I have already retried. |
# Conflicts: # exporters/otlp/otlplog/otlploggrpc/client_test.go
b5288ca to
b778294
Compare
|
This PR is expected to wait until after version 1.38; we need to wait to avoid blocking the 1.38 release. |
OK |
There was a problem hiding this comment.
This is a high-quality PR. Thank you for contributing it and thank you for following the contribution guidelines.
There are some minor naming issues that need to be addressed. But there are two larger issues that still need attention:
- There are no benchmark tests showing the performance of the
Instrumentationmethods - The performance of adding the RPC status code attribute needs to be better evaluated
| // | ||
| // The number of successful exports is provided as success. Any error that is encountered is provided as error | ||
| // The code of last gRPC requests performed in scope of this export call. | ||
| type ExportSpanDone func(err error, success int64, code codes.Code) |
There was a problem hiding this comment.
| type ExportSpanDone func(err error, success int64, code codes.Code) | |
| type ExportLogsDone func(err error, success int64, code codes.Code) |
Docs will need an update as well.
|
|
||
| // ExportSpans instruments the ExportSpans method of the exporter. It returns a | ||
| // function that needs to be deferred so it is called when the method returns. | ||
| func (i *Instrumentation) ExportSpans(ctx context.Context, count int64) ExportSpanDone { |
There was a problem hiding this comment.
| func (i *Instrumentation) ExportSpans(ctx context.Context, count int64) ExportSpanDone { | |
| func (i *Instrumentation) ExportLogs(ctx context.Context, count int64) ExportLogsDone { |
Docs will need an update as well.
|
|
||
| logExportedMetric, e := otelconv.NewSDKExporterLogExported(m) | ||
| if e != nil { | ||
| e = fmt.Errorf("failed to create span exported metric: %w", e) |
There was a problem hiding this comment.
| e = fmt.Errorf("failed to create span exported metric: %w", e) | |
| e = fmt.Errorf("failed to create log exported metric: %w", e) |
|
|
||
| logInflightMetric, e := otelconv.NewSDKExporterLogInflight(m) | ||
| if e != nil { | ||
| e = fmt.Errorf("failed to create span inflight metric: %w", e) |
There was a problem hiding this comment.
| e = fmt.Errorf("failed to create span inflight metric: %w", e) | |
| e = fmt.Errorf("failed to create log inflight metric: %w", e) |
| // NewInstrumentation returns instrumentation for otlplog grpc exporter. | ||
| func NewInstrumentation( | ||
| name, componentName string, | ||
| componentType otelconv.ComponentTypeAttr, |
There was a problem hiding this comment.
This will always be otelconv.ComponentTypeOtlpGRPCLogExporter right? Do we need to accept it as a parameter?
There was a problem hiding this comment.
Yes, I will adjust it later.
|
Because this process involves some specification adjustments and historical reviews (which may even include invalid review suggestions), the information of other approvers can no longer be viewed during the review, causing some inconvenience. Could we create a new PR based on the current branch to reduce distracting information. |
|
Sure. I will open a new PR based on it later. |
| // | ||
| // This package is used to generate unique IDs while allowing testing packages | ||
| // to reset the counter. | ||
| package counter // import "go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/internal/counter" |
There was a problem hiding this comment.
Since we already have this template, we can consider using it for generation to ensure unified maintenance. https://github.com/open-telemetry/opentelemetry-go/tree/main/internal/shared/counter
|
|
||
| - Add `WithInstrumentationAttributeSet` option to `go.opentelemetry.io/otel/log`, `go.opentelemetry.io/otel/metric`, and `go.opentelemetry.io/otel/trace` packages. | ||
| This provides a concurrent-safe and performant alternative to `WithInstrumentationAttributes` by accepting a pre-constructed `attribute.Set`. (#7287) | ||
| - - Add experimental self-observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. |
There was a problem hiding this comment.
| - - Add experimental self-observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. | |
| - Add experimental self-observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. |
| logInflightMetric, e := otelconv.NewSDKExporterLogInflight(m) | ||
| if e != nil { | ||
| e = fmt.Errorf("failed to create span inflight metric: %w", e) | ||
| otel.Handle(e) |
There was a problem hiding this comment.
Since we’re returning the err, I’m thinking we can skip logging this part and let the outer caller decide uniformly.
The same applies to the others.
There was a problem hiding this comment.
good suggestion, this error should be handled by the caller.
|
@open-telemetry/go-approvers , all done, I create a new PR based on the current branch , place check. Thank you all for your suggestions on this PR. ❤️ |
Since a new one has been created, we can close the current PR. |
|
done. |
This PR adds support for experimental metrics in `otlploggrpc` - `otel.sdk.exporter.log.inflight` - `otel.sdk.exporter.log.exported` - `otel.sdk.exporter.operation.duration` References: - #7084 - #7019 - [Follow guidelines](https://github.com/open-telemetry/opentelemetry-go/blob/a5dcd68ebb2f3669f7685ac7b0f3f1624251a381/CONTRIBUTING.md#encapsulation). ----- ```txt goos: darwin goarch: arm64 pkg: go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc cpu: Apple M3 │ disabled.txt │ enabled.txt │ │ sec/op │ sec/op vs base │ ExporterExportLogs/Observability-8 681.5µ ± 3% 684.3µ ± 6% ~ (p=0.315 n=10) │ disabled.txt │ enabled.txt │ │ B/op │ B/op vs base │ ExporterExportLogs/Observability-8 672.8Ki ± 0% 673.6Ki ± 1% ~ (p=0.247 n=10) │ disabled.txt │ enabled.txt │ │ allocs/op │ allocs/op vs base │ ExporterExportLogs/Observability-8 9.224k ± 0% 9.232k ± 0% +0.09% (p=0.000 n=10) ``` ----- ```txt goos: darwin goarch: arm64 pkg: go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/internal/observ cpu: Apple M3 │ bench.txt │ │ sec/op │ InstrumentationExportLogs/NoError-8 162.6n ± 3% InstrumentationExportLogs/PartialError-8 705.5n ± 5% InstrumentationExportLogs/FullError-8 592.1n ± 1% geomean 408.0n │ bench.txt │ │ B/op │ InstrumentationExportLogs/NoError-8 152.0 ± 0% InstrumentationExportLogs/PartialError-8 697.0 ± 0% InstrumentationExportLogs/FullError-8 616.0 ± 0% geomean 402.6 │ bench.txt │ │ allocs/op │ InstrumentationExportLogs/NoError-8 3.000 ± 0% InstrumentationExportLogs/PartialError-8 10.00 ± 0% InstrumentationExportLogs/FullError-8 8.000 ± 0% geomean 6.214 ``` ----- ```txt pkg: go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/internal/observ cpu: Apple M3 │ parse_target.txt │ │ sec/op │ ParseTarget/HostName-8 38.00n ± ∞ ¹ ParseTarget/HostPort-8 51.33n ± ∞ ¹ ParseTarget/IPv4WithoutPort-8 44.74n ± ∞ ¹ ParseTarget/IPv4WithPort-8 62.56n ± ∞ ¹ ParseTarget/IPv6Bare-8 94.89n ± ∞ ¹ ParseTarget/IPv6Bracket-8 93.78n ± ∞ ¹ ParseTarget/IPv6WithPort-8 57.57n ± ∞ ¹ ParseTarget/UnixSocket-8 8.329n ± ∞ ¹ ParseTarget/UnixAbstractSocket-8 9.082n ± ∞ ¹ ParseTarget/Passthrough-8 58.06n ± ∞ ¹ geomean 40.64n ¹ need >= 6 samples for confidence interval at level 0.95 │ parse_target.txt │ │ B/op │ ParseTarget/HostName-8 48.00 ± ∞ ¹ ParseTarget/HostPort-8 48.00 ± ∞ ¹ ParseTarget/IPv4WithoutPort-8 16.00 ± ∞ ¹ ParseTarget/IPv4WithPort-8 48.00 ± ∞ ¹ ParseTarget/IPv6Bare-8 16.00 ± ∞ ¹ ParseTarget/IPv6Bracket-8 16.00 ± ∞ ¹ ParseTarget/IPv6WithPort-8 48.00 ± ∞ ¹ ParseTarget/UnixSocket-8 0.000 ± ∞ ¹ ParseTarget/UnixAbstractSocket-8 0.000 ± ∞ ¹ ParseTarget/Passthrough-8 48.00 ± ∞ ¹ geomean ² ¹ need >= 6 samples for confidence interval at level 0.95 ² summaries must be >0 to compute geomean │ parse_target.txt │ │ allocs/op │ ParseTarget/HostName-8 1.000 ± ∞ ¹ ParseTarget/HostPort-8 1.000 ± ∞ ¹ ParseTarget/IPv4WithoutPort-8 1.000 ± ∞ ¹ ParseTarget/IPv4WithPort-8 1.000 ± ∞ ¹ ParseTarget/IPv6Bare-8 1.000 ± ∞ ¹ ParseTarget/IPv6Bracket-8 1.000 ± ∞ ¹ ParseTarget/IPv6WithPort-8 1.000 ± ∞ ¹ ParseTarget/UnixSocket-8 0.000 ± ∞ ¹ ParseTarget/UnixAbstractSocket-8 0.000 ± ∞ ¹ ParseTarget/Passthrough-8 1.000 ± ∞ ¹ geomean ² ¹ need >= 6 samples for confidence interval at level 0.95 ² summaries must be >0 to compute geomean ``` --------- Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
fix #7019