-
Notifications
You must be signed in to change notification settings - Fork 1.2k
sdk/log: move Enabled method from FilterProcessor to Processor #7639
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 all commits
fccca34
b83a30e
988eeae
234241d
c360f38
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 |
|---|---|---|
|
|
@@ -131,15 +131,15 @@ func BenchmarkProcessor(b *testing.B) { | |
|
|
||
| type timestampProcessor struct{} | ||
|
|
||
| func (timestampProcessor) Enabled(context.Context, EnabledParameters) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (timestampProcessor) OnEmit(_ context.Context, r *Record) error { | ||
| r.SetObservedTimestamp(time.Date(1988, time.November, 17, 0, 0, 0, 0, time.UTC)) | ||
| return nil | ||
| } | ||
|
|
||
| func (timestampProcessor) Enabled(context.Context, Record) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (timestampProcessor) Shutdown(context.Context) error { | ||
| return nil | ||
| } | ||
|
|
@@ -150,15 +150,15 @@ func (timestampProcessor) ForceFlush(context.Context) error { | |
|
|
||
| type attrAddProcessor struct{} | ||
|
|
||
| func (attrAddProcessor) Enabled(context.Context, EnabledParameters) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (attrAddProcessor) OnEmit(_ context.Context, r *Record) error { | ||
| r.AddAttributes(log.String("add", "me")) | ||
| return nil | ||
| } | ||
|
|
||
| func (attrAddProcessor) Enabled(context.Context, Record) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (attrAddProcessor) Shutdown(context.Context) error { | ||
| return nil | ||
| } | ||
|
|
@@ -169,15 +169,15 @@ func (attrAddProcessor) ForceFlush(context.Context) error { | |
|
|
||
| type attrSetDecorator struct{} | ||
|
|
||
| func (attrSetDecorator) Enabled(context.Context, EnabledParameters) bool { | ||
|
Contributor
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. Nit: moving this up 7 lines creates unnecessary diffs.
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. Ditto |
||
| return true | ||
| } | ||
|
|
||
| func (attrSetDecorator) OnEmit(_ context.Context, r *Record) error { | ||
| r.SetAttributes(log.String("replace", "me")) | ||
| return nil | ||
| } | ||
|
|
||
| func (attrSetDecorator) Enabled(context.Context, Record) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (attrSetDecorator) Shutdown(context.Context) error { | ||
| return nil | ||
| } | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,16 +5,46 @@ package log // import "go.opentelemetry.io/otel/sdk/log" | |
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "go.opentelemetry.io/otel/log" | ||
| "go.opentelemetry.io/otel/sdk/instrumentation" | ||
| ) | ||
|
|
||
| // Processor handles the processing of log records. | ||
| // | ||
| // Any of the Processor's methods may be called concurrently with itself | ||
| // or with other methods. It is the responsibility of the Processor to manage | ||
| // this concurrency. | ||
| // | ||
| // See [FilterProcessor] for information about how a Processor can support filtering. | ||
| type Processor interface { | ||
| // Enabled reports whether the Processor will process for the given context | ||
| // and param. | ||
| // | ||
| // The passed param is likely to be partial record information being | ||
| // provided (e.g. a param with only the Severity set). | ||
|
Comment on lines
+22
to
+23
Contributor
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. This comment text seems to apply more to the previous version.
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. Which is still partial record information. This may contain more fields in the future. Still we can rephrase it if you have a proposal. Yet, I prefer doing it in a separate PR. This PR tries to keep the existing documentation. |
||
| // If a Processor needs more information than is provided, it | ||
| // is said to be in an indeterminate state (see below). | ||
| // | ||
| // The returned value will be true when the Processor will process for the | ||
| // provided context and param, and will be false if the Processor will not | ||
| // process. The returned value may be true or false in an indeterminate state. | ||
| // An implementation should default to returning true for an indeterminate | ||
| // state, but may return false if valid reasons in particular circumstances | ||
| // exist (e.g. performance, correctness). | ||
|
Comment on lines
+24
to
+32
Contributor
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 can't see why this belongs as documentation in the interface. What would I as a user do based on this information?
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. This is for users implementing this interface. I will try rephrasing but I prefer doing it in a separate PR. This PR tries to keep the existing documentation. |
||
| // | ||
| // The param should not be held by the implementation. A copy should be | ||
| // made if the param needs to be held after the call returns. | ||
| // | ||
| // Processor implementations are expected to re-evaluate the [Record] passed | ||
| // to OnEmit. It is not expected that the caller to OnEmit will | ||
| // use the result from Enabled prior to calling OnEmit. | ||
|
Comment on lines
+34
to
+39
Contributor
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. This all seems to apply only to the previous implementation?
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. The implementation has not changed. However, I think you are right that this comment is unnecessary (param is immutable). I will propose removing but I prefer doing it in a separate PR. This PR tries to keep the existing documentation. |
||
| // | ||
| // The SDK's Logger.Enabled returns false if all the registered processors | ||
| // return false. Otherwise, it returns true. | ||
|
Comment on lines
+41
to
+42
Contributor
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. Belongs with
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. The issue is that the Logger implantation is not exported so I am not sure where is a better place to put this information. Do you have any proposal? |
||
| // | ||
| // Implementations of this method need to be safe for a user to call | ||
| // concurrently. | ||
| Enabled(ctx context.Context, param EnabledParameters) bool | ||
|
|
||
| // OnEmit is called when a Record is emitted. | ||
| // | ||
| // OnEmit will be called independent of Enabled. Implementations need to | ||
|
|
@@ -32,7 +62,8 @@ type Processor interface { | |
| // they were registered using WithProcessor. | ||
| // Implementations may synchronously modify the record so that the changes | ||
| // are visible in the next registered processor. | ||
| // Notice that Record is not concurrent safe. Therefore, asynchronous | ||
| // | ||
| // Note that Record is not concurrent safe. Therefore, asynchronous | ||
| // processing may cause race conditions. Use Record.Clone | ||
| // to create a copy that shares no state with the original. | ||
| OnEmit(ctx context.Context, record *Record) error | ||
|
|
@@ -54,3 +85,10 @@ type Processor interface { | |
| // appropriate error should be returned in these situations. | ||
| ForceFlush(ctx context.Context) error | ||
| } | ||
|
|
||
| // EnabledParameters represents payload for [Processor]'s Enabled method. | ||
| type EnabledParameters struct { | ||
| InstrumentationScope instrumentation.Scope | ||
| Severity log.Severity | ||
| EventName string | ||
| } | ||
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.
Nit: moving this up 7 lines creates unnecessary diffs.
Uh oh!
There was an error while loading. Please reload this page.
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.
True. I wanted to keep consistency in the repo and have Enabled method before OnEmit. 234241d. Should I scope it to a separate PR?