-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Instrument the SimpleLogProcessor from sdk/log
#7548
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
0481fcb
402892c
66c7301
0788d3f
0541716
16b8496
8d4cd7f
ca0e076
a8fae0c
3b942bd
2bec291
9addd81
bd7e92f
53019cb
1c9ec32
b7e05a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,9 @@ package log // import "go.opentelemetry.io/otel/sdk/log" | |
| import ( | ||
| "context" | ||
| "sync" | ||
|
|
||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/sdk/log/internal/observ" | ||
| ) | ||
|
|
||
| // Compile-time check SimpleProcessor implements Processor. | ||
|
|
@@ -17,8 +20,8 @@ var _ Processor = (*SimpleProcessor)(nil) | |
| type SimpleProcessor struct { | ||
| mu sync.Mutex | ||
| exporter Exporter | ||
|
|
||
| noCmp [0]func() //nolint: unused // This is indeed used. | ||
| inst *observ.SLP | ||
| noCmp [0]func() //nolint: unused // This is indeed used. | ||
| } | ||
|
|
||
| // NewSimpleProcessor is a simple Processor adapter. | ||
|
|
@@ -30,7 +33,15 @@ type SimpleProcessor struct { | |
| // [NewBatchProcessor] instead. However, there may be exceptions where certain | ||
| // [Exporter] implementations perform better with this Processor. | ||
| func NewSimpleProcessor(exporter Exporter, _ ...SimpleProcessorOption) *SimpleProcessor { | ||
| return &SimpleProcessor{exporter: exporter} | ||
| slp := &SimpleProcessor{ | ||
| exporter: exporter, | ||
| } | ||
| var err error | ||
| slp.inst, err = observ.NewSLP(observ.NextSimpleProcessorID()) | ||
| if err != nil { | ||
| otel.Handle(err) | ||
| } | ||
| return slp | ||
| } | ||
|
|
||
| var simpleProcRecordsPool = sync.Pool{ | ||
|
|
@@ -41,7 +52,7 @@ var simpleProcRecordsPool = sync.Pool{ | |
| } | ||
|
|
||
| // OnEmit batches provided log record. | ||
| func (s *SimpleProcessor) OnEmit(ctx context.Context, r *Record) error { | ||
| func (s *SimpleProcessor) OnEmit(ctx context.Context, r *Record) (err error) { | ||
| if s.exporter == nil { | ||
| return nil | ||
| } | ||
|
|
@@ -55,6 +66,11 @@ func (s *SimpleProcessor) OnEmit(ctx context.Context, r *Record) error { | |
| simpleProcRecordsPool.Put(records) | ||
| }() | ||
|
|
||
| if s.inst != nil { | ||
| defer func() { | ||
| s.inst.LogProcessed(ctx, err) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking that when this part of the logic is triggered, this section should also be reported. if s.exporter == nil {
return nil
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @flc1125, I don't quite understand this part.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I want to express is that we should also report this metric regarding this part of the situation. However, what's a bit special here is that what is returned here is cc @MrAlias, if you have any good suggestions, please feel free to add them.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @flc1125 about this issue, maybe we could address this in an separate PR, WDYT?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we at least need to support indicators reporting for this scenario; the point of disagreement lies in the issue of if s.exporter == nil {
return nil
}Regarding the points of disagreement, I think it's fine to discuss them in another PR or issue. |
||
| }() | ||
| } | ||
| return s.exporter.Export(ctx, *records) | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.