Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add setStatus operation for spanprocessor #5886

Merged
merged 10 commits into from
Nov 29, 2021
Merged

Add setStatus operation for spanprocessor #5886

merged 10 commits into from
Nov 29, 2021

Conversation

TomRoSystems
Copy link
Member

Description: Added possibility to change span status and description.

Link to tracking Issue: #4754

Testing: Added two unit tests in span_test.go

Documentation: Updated README inside spanprocessor and added two examples with comments in config.yaml

@TomRoSystems TomRoSystems requested review from a team and mx-psi October 25, 2021 14:02
mx-psi
mx-psi requested changes Oct 25, 2021
View reviewed changes
processor/spanprocessor/config.go Outdated Show resolved Hide resolved
processor/spanprocessor/config.go Outdated Show resolved Hide resolved
processor/spanprocessor/factory.go Show resolved Hide resolved
processor/spanprocessor/factory.go Outdated Show resolved Hide resolved
processor/spanprocessor/README.md Outdated Show resolved Hide resolved
processor/spanprocessor/factory.go Outdated Show resolved Hide resolved
mx-psi
mx-psi reviewed Oct 26, 2021
View reviewed changes
processor/spanprocessor/config.go Outdated Show resolved Hide resolved
processor/spanprocessor/factory.go Outdated Show resolved Hide resolved
processor/spanprocessor/span.go Show resolved Hide resolved
mx-psi
mx-psi reviewed Oct 27, 2021
View reviewed changes
processor/spanprocessor/config.go Outdated Show resolved Hide resolved
processor/spanprocessor/config.go Outdated Show resolved Hide resolved
processor/spanprocessor/testdata/config.yaml Outdated Show resolved Hide resolved
mx-psi
mx-psi reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just one more thing to fix from my side

processor/spanprocessor/span.go Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Oct 28, 2021

@iNikem @johnbley would you mind reviewing the PR since you commented on it?

johnbley
johnbley approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Nov 5, 2021
bogdandrutu
bogdandrutu requested changes Nov 5, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we need to finalize the transform processor configs before adding new functionality

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 13, 2021
@iNikem
Copy link
Contributor

iNikem commented Nov 13, 2021

@bogdandrutu Are there any delivery estimates for that pre-requirement?

@github-actions github-actions bot removed the Stale label Nov 14, 2021
@TomRoSystems
Copy link
Member Author

Sorry, we need to finalize the transform processor configs before adding new functionality

Hi @bogdandrutu

Could you please link corresponding issue that should be done first?

@bogdandrutu
Copy link
Member

Here is the design doc open-telemetry/opentelemetry-collector#4444

@bogdandrutu bogdandrutu removed the ready to merge Code review completed; ready to merge by maintainers label Nov 19, 2021
@iNikem
Copy link
Contributor

iNikem commented Nov 22, 2021

One more use-case for this PR is if somebody is unhappy with open-telemetry/opentelemetry-specification#1998, then using this new processor they can set status back to error

bogdandrutu
bogdandrutu approved these changes Nov 29, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the recent spec change we need this functionality before the stable transform processor is designed.

@TomRoSystems @iNikem fyi this functionality will be deprecated soon (and replace by the transform processor), so consider to not use it unless critical.

@bogdandrutu bogdandrutu merged commit 86a4e93 into open-telemetry:main Nov 29, 2021
@iNikem
Copy link
Contributor

iNikem commented Nov 30, 2021

Because of the recent spec change we need this functionality before the stable transform processor is designed.

@TomRoSystems @iNikem fyi this functionality will be deprecated soon (and replace by the transform processor), so consider to not use it unless critical.

@bogdandrutu is there some description/issue/anything about this "transform processor"?

@ivomagi
Copy link

ivomagi commented Nov 30, 2021

+1, can we get some transparency to expected transform processor design and the any estimates on release date / deprecation date? There are several asks for the possibility to get the possibility for overriding the span status set by instrumentations and looking for guidance how to enable such use cases while taking the upgrade path into account @bogdandrutu

@mx-psi
Copy link
Member

mx-psi commented Dec 1, 2021

@iNikem @ivomagi IIUC this is the design doc for that processor #5886 (comment)

jamesmoessis pushed a commit to atlassian-forks/opentelemetry-collector-contrib that referenced this pull request Dec 8, 2021
@TomRoSystems @mx-psi @jamesmoessis
Add setStatus operation for spanprocessor (open-telemetry#5886)
959f6f4 
* Add setStatus operation for spanprocessor

* Change code to have text format instead of numeric and add another example

* Add "Unset" value and tests

* Update README for Unset value

* Apply suggestions from code review

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>

* From review: Move statuses names to constants and fix "Err" => "Error"

* Code review: fixing style

* One more "Err" and fixing comment(s) as suggested from review

* Clear status description for cases different than error

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@iNikem iNikem mentioned this pull request Jan 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem left review comments

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@johnbley johnbley johnbley approved these changes

@mx-psi mx-psi mx-psi approved these changes

@dmitryax dmitryax dmitryax approved these changes

Assignees

@dmitryax dmitryax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@TomRoSystems @mx-psi @iNikem @bogdandrutu @ivomagi @johnbley @dmitryax