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.

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

processors/baggage: add baggage span processor #5404

Merged
merged 26 commits into from
May 6, 2024

Conversation

codeboten
Copy link
Contributor

Fixes #5397

@codeboten codeboten requested a review from a team April 18, 2024 20:03
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
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
@pellared
Copy link
Member

pellared commented Apr 19, 2024

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
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.

Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
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
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
Co-authored-by: Damien Mathieu <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 22, 2024

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

@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.

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?

@MikeGoldsmith
Copy link
Member

MikeGoldsmith commented May 3, 2024

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
* processors/baggage: add baggage span processor

Fixes open-telemetry#5397

Signed-off-by: Alex Boten <[email protected]>

* fix lint issues + dependabot generate

Signed-off-by: Alex Boten <[email protected]>

* apply review feedback

Signed-off-by: Alex Boten <[email protected]>

* more review feedback

Signed-off-by: Alex Boten <[email protected]>

* Update processors/baggage/processor_test.go

Co-authored-by: Robert Pająk <[email protected]>

* lint

Signed-off-by: Alex Boten <[email protected]>

* feedback from review

Signed-off-by: Alex Boten <[email protected]>

* rename baggage -> baggagetrace

Signed-off-by: Alex Boten <[email protected]>

* more fixes

Signed-off-by: Alex Boten <[email protected]>

* docs

Signed-off-by: Alex Boten <[email protected]>

* lint fixes

Signed-off-by: Alex Boten <[email protected]>

* generate

Signed-off-by: Alex Boten <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Damien Mathieu <[email protected]>

* Update CHANGELOG.md

* Update processors/baggage/baggagetrace/doc.go

* Apply suggestions from code review

Co-authored-by: Robert Pająk <[email protected]>

* Update processors/baggage/baggagetrace/processor.go

Co-authored-by: Robert Pająk <[email protected]>

* Apply suggestions from code review

* Update CHANGELOG.md

* Update CODEOWNERS

Co-authored-by: Mike Goldsmith <[email protected]>

---------

Signed-off-by: Alex Boten <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Mike Goldsmith <[email protected]>
Co-authored-by: Aaron Clawson <[email protected]>
MrAlias added a commit that referenced this pull request May 21, 2024
### Added

- Add the new `go.opentelemetry.io/contrib/instrgen` package to provide
auto-generated source code instrumentation. (#3068, #3108)
- Add an experimental `OTEL_METRICS_PRODUCERS` environment variable to
`go.opentelemetry.io/contrib/autoexport` to be set metrics producers.
(#5281)
- `prometheus` and `none` are supported values. You can specify multiple
producers separated by a comma.
- Add `WithFallbackMetricProducer` option that adds a fallback if the
`OTEL_METRICS_PRODUCERS` is not set or empty.
- The `go.opentelemetry.io/contrib/processors/baggage/baggagetrace`
module. This module provides a Baggage Span Processor. (#5404)
- Add gRPC trace `Filter` for stats handler to
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`.
(#5196)
- Add a repository Code Ownership Policy. (#5555)
- The `go.opentelemetry.io/contrib/bridges/otellogrus` module. This
module provides an OpenTelemetry logging bridge for
`github.com/sirupsen/logrus`. (#5355)
- The `WithVersion` option function in
`go.opentelemetry.io/contrib/bridges/otelslog`. This option function is
used as a replacement of `WithInstrumentationScope` to specify the
logged package version. (#5588)
- The `WithSchemaURL` option function in
`go.opentelemetry.io/contrib/bridges/otelslog`. This option function is
used as a replacement of `WithInstrumentationScope` to specify the
semantic convention schema URL for the logged records. (#5588)
- Add support for Cloud Run jobs in
`go.opentelemetry.io/contrib/detectors/gcp`. (#5559)

### Changed

- The gRPC trace `Filter` for interceptor is renamed to
`InterceptorFilter`. (#5196)
- The gRPC trace filter functions `Any`, `All`, `None`, `Not`,
`MethodName`, `MethodPrefix`, `FullMethodName`, `ServiceName`,
`ServicePrefix` and `HealthCheck` for interceptor are moved to
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/filters/interceptor`.
With this change, the filters in
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`
are now working for stats handler. (#5196)

- `NewLogger` now accepts a `name` `string` as the first argument. This
parameter is used as a replacement of `WithInstrumentationScope` to
specify the name of the logger backing the underlying `Handler`. (#5588)
- `NewHandler` now accepts a `name` `string` as the first argument. This
parameter is used as a replacement of `WithInstrumentationScope` to
specify the name of the logger backing the returned `Handler`. (#5588)
- Upgrade all dependencies of `go.opentelemetry.io/otel/semconv/v1.24.0`
to `go.opentelemetry.io/otel/semconv/v1.25.0`. (#5605)

### Removed

- The `WithInstrumentationScope` option function in
`go.opentelemetry.io/contrib/bridges/otelslog` is removed. Use the
`name` parameter added to `NewHandler` and `NewLogger` as well as
`WithVersion` and `WithSchema` as replacements. (#5588)

### Deprecated

- The `InterceptorFilter` type in
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`
is deprecated. (#5196)
khushijain21 pushed a commit to khushijain21/opentelemetry-go-contrib that referenced this pull request May 22, 2024
### Added

- Add the new `go.opentelemetry.io/contrib/instrgen` package to provide
auto-generated source code instrumentation. (open-telemetry#3068, open-telemetry#3108)
- Add an experimental `OTEL_METRICS_PRODUCERS` environment variable to
`go.opentelemetry.io/contrib/autoexport` to be set metrics producers.
(open-telemetry#5281)
- `prometheus` and `none` are supported values. You can specify multiple
producers separated by a comma.
- Add `WithFallbackMetricProducer` option that adds a fallback if the
`OTEL_METRICS_PRODUCERS` is not set or empty.
- The `go.opentelemetry.io/contrib/processors/baggage/baggagetrace`
module. This module provides a Baggage Span Processor. (open-telemetry#5404)
- Add gRPC trace `Filter` for stats handler to
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`.
(open-telemetry#5196)
- Add a repository Code Ownership Policy. (open-telemetry#5555)
- The `go.opentelemetry.io/contrib/bridges/otellogrus` module. This
module provides an OpenTelemetry logging bridge for
`github.com/sirupsen/logrus`. (open-telemetry#5355)
- The `WithVersion` option function in
`go.opentelemetry.io/contrib/bridges/otelslog`. This option function is
used as a replacement of `WithInstrumentationScope` to specify the
logged package version. (open-telemetry#5588)
- The `WithSchemaURL` option function in
`go.opentelemetry.io/contrib/bridges/otelslog`. This option function is
used as a replacement of `WithInstrumentationScope` to specify the
semantic convention schema URL for the logged records. (open-telemetry#5588)
- Add support for Cloud Run jobs in
`go.opentelemetry.io/contrib/detectors/gcp`. (open-telemetry#5559)

### Changed

- The gRPC trace `Filter` for interceptor is renamed to
`InterceptorFilter`. (open-telemetry#5196)
- The gRPC trace filter functions `Any`, `All`, `None`, `Not`,
`MethodName`, `MethodPrefix`, `FullMethodName`, `ServiceName`,
`ServicePrefix` and `HealthCheck` for interceptor are moved to
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/filters/interceptor`.
With this change, the filters in
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`
are now working for stats handler. (open-telemetry#5196)

- `NewLogger` now accepts a `name` `string` as the first argument. This
parameter is used as a replacement of `WithInstrumentationScope` to
specify the name of the logger backing the underlying `Handler`. (open-telemetry#5588)
- `NewHandler` now accepts a `name` `string` as the first argument. This
parameter is used as a replacement of `WithInstrumentationScope` to
specify the name of the logger backing the returned `Handler`. (open-telemetry#5588)
- Upgrade all dependencies of `go.opentelemetry.io/otel/semconv/v1.24.0`
to `go.opentelemetry.io/otel/semconv/v1.25.0`. (open-telemetry#5605)

### Removed

- The `WithInstrumentationScope` option function in
`go.opentelemetry.io/contrib/bridges/otelslog` is removed. Use the
`name` parameter added to `NewHandler` and `NewLogger` as well as
`WithVersion` and `WithSchema` as replacements. (open-telemetry#5588)

### Deprecated

- The `InterceptorFilter` type in
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`
is deprecated. (open-telemetry#5196)
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for new component: Baggage Span Processor
6 participants