-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add setStatus operation for spanprocessor #1
Conversation
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.
Conceptually looks good to me.
You can also borrow the matching conditions from filterprocessor have have a apply_to
subsection where you specify which spans to apply the changes to. filterprocessor currently only supports conditions based on attributes, not sure how useful it is as a condition for setting the status.
Also keep in mind there is an initiative to overhaul the processors: https://docs.google.com/document/d/1UNzMG1LBUWpiMiFn1PX4OPVVQsk6EneISbtR4cdBm4Y/edit#
However it is not approved and it is not clear if it will be approved.
processor/spanprocessor/README.md
Outdated
# Set status allows to set specific status for a given span. | ||
span/set_status: | ||
status: | ||
code: 2 |
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.
It would be nicer to use symbolic status code names (Unset, Ok, Error) instead of numeric codes.
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.
As suggested - please take a look.
Co-authored-by: Pablo Baeyens <[email protected]>
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.
LGTM, some minor comments.
processor/spanprocessor/factory.go
Outdated
@@ -35,7 +35,8 @@ var processorCapabilities = consumer.Capabilities{MutatesData: true} | |||
// is not specified. | |||
// TODO https://github.com/open-telemetry/opentelemetry-collector/issues/215 | |||
// Move this to the error package that allows for span name and field to be specified. | |||
var errMissingRequiredField = errors.New("error creating \"span\" processor: either \"from_attributes\" or \"to_attributes\" must be specified in \"name:\"") | |||
var errMissingRequiredField = errors.New("error creating \"span\" processor: either \"from_attributes\" or \"to_attributes\" must be specified in \"name:\" or \"setStatus\" must be specified") |
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: using back quote strings may make this more readable.
processor/spanprocessor/factory.go
Outdated
return nil, errMissingRequiredField | ||
} | ||
|
||
if oCfg.SetStatus != nil && | ||
(oCfg.SetStatus.Code != "Ok" && oCfg.SetStatus.Code != "Err" && oCfg.SetStatus.Code != "Unset") { |
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: would be nice to define "Ok", "Err", "Unsert" as consts and use everywhere.
# The description field allows to set a human-readable message for errors. | ||
span/set_status: | ||
status: | ||
code: Error |
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.
Is it Err or Error? Make sure to match Otel spec.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
Adding possibility to change status of a span.
Link to tracking Issue: open-telemetry#4754
Testing: Added unit test to check functionality.
Documentation: Description in spanprocessor readme was extened along with an example.