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

processors/baggage: add baggage span processor #5404

Merged
merged 26 commits into from May 6, 2024
Merged

processors/baggage: add baggage span processor #5404

merged 26 commits into from May 6, 2024

Conversation

codeboten
Copy link
Contributor

Fixes #5397

@codeboten
processors/baggage: add baggage span processor
9f17aed 
Fixes open-telemetry#5397

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
fix lint issues + dependabot generate
33e94b2 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
pellared
pellared reviewed Apr 18, 2024
View reviewed changes
processors/baggage/processor_test.go Outdated Show resolved Hide resolved
processors/baggage/processor_test.go Outdated Show resolved Hide resolved
processors/baggage/processor_test.go Outdated Show resolved Hide resolved
processors/baggage/processor_test.go Outdated Show resolved Hide resolved
processors/baggage/processor_test.go Outdated Show resolved Hide resolved
processors/baggage/processor_test.go Outdated Show resolved Hide resolved
@codeboten
Copy link
Contributor Author

Thanks for the quick review @pellared! Will address your comments shortly

codeboten and others added 4 commits April 18, 2024 14:52
@codeboten
apply review feedback
b65b129 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
more review feedback
cfa0e23 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten @pellared
Update processors/baggage/processor_test.go
8e648db 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@codeboten
lint
9ed5020 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@pellared
Copy link
Member

pellared commented Apr 19, 2024 •
edited

Can you also update the README.md? I see this is the first processor. Shouldn't we have a "split" for trace and log processors? I think we could have a similar log record processor.

@codeboten
feedback from review
fa28a2f 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
rename baggage -> baggagetrace
26a0d60 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
Copy link
Contributor Author

@pellared i've updated the readme and moved the contents of the baggage package for traces under baggage/baggagetrace to follow the exporters pattern in the go repo.

@codeboten
more fixes
1a79494 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
docs
9bd6d4e 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
lint fixes
5e10970 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
generate
2d8ed5d 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
dmathieu
dmathieu reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

How about adding an example_test?

CHANGELOG.md Outdated Show resolved Hide resolved
pellared
pellared reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Just nit comments to address

Overall, LGTM

processors/baggage/baggagetrace/doc.go Outdated Show resolved Hide resolved
@codeboten @dmathieu
Update CHANGELOG.md
e689203 
Co-authored-by: Damien Mathieu <42@dmathieu.com>
codeboten
codeboten commented Apr 22, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pellared
pellared reviewed Apr 22, 2024
View reviewed changes
processors/baggage/baggagetrace/go.mod Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pellared
pellared reviewed Apr 22, 2024
View reviewed changes
processors/baggage/baggagetrace/processor.go Outdated Show resolved Hide resolved
processors/baggage/baggagetrace/doc.go Outdated Show resolved Hide resolved
codeboten
codeboten commented Apr 22, 2024
View reviewed changes
CODEOWNERS Outdated Show resolved Hide resolved
codeboten and others added 2 commits April 22, 2024 08:10
@codeboten @pellared
Apply suggestions from code review
d710414 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@codeboten @pellared
Update processors/baggage/baggagetrace/processor.go
85ecca9 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@codecov Codecov
Copy link

codecov bot commented Apr 22, 2024 •
edited

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 62.4%. Comparing base (f251adc) to head (407c7df).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5404   +/-   ##
=====================================
  Coverage   62.4%   62.4%           
=====================================
  Files        189     190    +1     
  Lines      11638   11646    +8     
=====================================
+ Hits        7263    7271    +8     
- Misses      4158    4159    +1     
+ Partials     217     216    -1
Files Coverage Δ
processors/baggage/baggagetrace/processor.go 75.0% <75.0%> (ø)

... and 1 file with indirect coverage changes

@MikeGoldsmith MikeGoldsmith mentioned this pull request Apr 23, 2024
Closed
@MikeGoldsmith MikeGoldsmith mentioned this pull request Apr 25, 2024
Merged
3 tasks
@codeboten
Copy link
Contributor Author

I see this is approved, was there more work needed before merging this?

@pellared
Copy link
Member

I see this is approved, was there more work needed before merging this?

It was discussed during last SIG meetings. We just want to have more reviews.

@MikeGoldsmith MikeGoldsmith mentioned this pull request Apr 29, 2024
Open
MikeGoldsmith
MikeGoldsmith reviewed May 2, 2024
View reviewed changes
CODEOWNERS Outdated Show resolved Hide resolved
@MikeGoldsmith
Copy link
Member

I see this is approved, was there more work needed before merging this?

It was discussed during last SIG meetings. We just want to have more reviews.

Hey @pellared - what is the best way to get more community reviews? This PR has been open for more than two weeks and only a couple of approvers have provided feedback.

Do you have a sense how many reviews and/or how long you'd want to wait for additional reviews before accepting?

Is it specific people whom you would like to provide feedback?

dmathieu and others added 2 commits May 2, 2024 14:06
@dmathieu @MikeGoldsmith
Update CODEOWNERS
7211096 
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
@codeboten
Merge branch 'main' into codeboten/processor-baggage
fa1d812
@MikeGoldsmith
Copy link
Member

MikeGoldsmith commented May 3, 2024 •
edited

FYI the CI failure is from codecov being rate limited:

debug - 2024-05-02 15:31:38,221 -- Commit creating result --- {"result": "RequestResult(error=RequestError(code='HTTP Error 429', params={}, description='{\"detail\":\"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 491 seconds.\"}'), warnings=[], status_code=429, text='{\"detail\":\"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 491 seconds.\"}')"}
error - 2024-05-02 15:31:38,221 -- Commit creating failed: {"detail":"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 491 seconds."}

@pellared
Copy link
Member

pellared commented May 6, 2024

@MrAlias, if I remember correctly you wanted to review this PR as well.

@MrAlias MrAlias merged commit 31c8398 into open-telemetry:main May 6, 2024
23 checks passed
zailic pushed a commit to zailic/opentelemetry-go-contrib that referenced this pull request May 7, 2024
@codeboten @pellared
@dmathieu @MikeGoldsmith @MadVikingGod @zailic
processors/baggage: add baggage span processor (open-telemetry#5404)
726a0bf 
* processors/baggage: add baggage span processor

Fixes open-telemetry#5397

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* fix lint issues + dependabot generate

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* apply review feedback

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* more review feedback

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* Update processors/baggage/processor_test.go

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* lint

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* feedback from review

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* rename baggage -> baggagetrace

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* more fixes

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* docs

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* lint fixes

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* generate

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: Damien Mathieu <42@dmathieu.com>

* Update CHANGELOG.md

* Update processors/baggage/baggagetrace/doc.go

* Apply suggestions from code review

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Update processors/baggage/baggagetrace/processor.go

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Apply suggestions from code review

* Update CHANGELOG.md

* Update CODEOWNERS

Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>

---------

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeGoldsmith MikeGoldsmith MikeGoldsmith approved these changes

@dmathieu dmathieu dmathieu approved these changes

@MadVikingGod MadVikingGod MadVikingGod approved these changes

@pellared pellared pellared approved these changes

@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request for new component: Baggage Span Processor
6 participants
@codeboten @pellared @MikeGoldsmith @dmathieu @MadVikingGod @MrAlias