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

Removed dropped link's attributes field from API package #2118

Conversation

nelsonghezzi
Copy link
Contributor

This PR removes the DroppedAttributeCount field from the otel/trace.Link struct in order to not leak implementation details into the API package.

A new trace.Link type is introduced in the otel/sdk module for the purpose of counting the number of dropped link's attributes.

The interface for adding links to a Span remains under the original otel/trace.Link type, but are then converted into tracesdk.Links for internal storage. They are also used to create ReadOnlySpans.

Fixes #2051.

@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #2118 (94a9eff) into main (03902d9) will increase coverage by 0.0%.
The diff coverage is 75.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2118   +/-   ##
=====================================
  Coverage   73.5%   73.5%           
=====================================
  Files        168     168           
  Lines       8233    8234    +1     
=====================================
+ Hits        6058    6059    +1     
  Misses      1935    1935           
  Partials     240     240           
Impacted Files Coverage Δ
...ters/otlp/otlptrace/internal/otlptracetest/data.go 0.0% <0.0%> (ø)
...ers/otlp/otlptrace/internal/tracetransform/span.go 96.3% <ø> (ø)
sdk/trace/snapshot.go 52.9% <ø> (ø)
trace/trace.go 98.3% <ø> (ø)
sdk/trace/span.go 89.1% <83.3%> (+<0.1%) ⬆️
sdk/trace/tracetest/span.go 96.7% <100.0%> (ø)

CHANGELOG.md Outdated
@@ -16,6 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Changed

- The `SpanModels` function is now exported from the `go.opentelemetry.io/otel/exporters/zipkin` package to convert OpenTelemetry spans into Zipkin model spans. (#2027)
- Replaced usages of the `trace.Link` type in the SDK package with an equivalent `Link` struct that also counts the number of dropped link's attributes. (#2118)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rewrite this from the user perspective? Likely this needs to be transformed into an item in the Add section describing the new type used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I've stretched it too far, but from a user/usage perspective, I can think in something like:

Added a new Link type under the SDK otel/sdk/trace package that counts the number of attributes that were dropped for surpassing the AttributePerLinkCountLimit configured in the Span's SpanLimits. This new type replaces the equal-named API Link type found in the otel/trace package for most usages within the SDK. For example, instances of this type are now returned by the Links() function of ReadOnlySpans provided in places like the OnEnd function of SpanProcessor implementations.

But don't hesitate on suggesting a better/shorter alternative if you had something in mind!

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@MrAlias
Copy link
Contributor

MrAlias commented Jul 23, 2021

This PR looks ready to be moved out of draft state.

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.

I added two nit comments, but for me, it is also fine as it is 😉

@@ -15,6 +15,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Added the `WithRetry` option to the `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp` package.
This option is a replacement for the removed `WithMaxAttempts` and `WithBackoff` options. (#2095)
- Added API `LinkFromContext` to return Link which encapsulates SpanContext from provided context and also encapsulates attributes. (#2115)
- Added a new `Link` type under the SDK `otel/sdk/trace` package that counts the number of attributes that were dropped for surpassing the `AttributePerLinkCountLimit` configured in the Span's `SpanLimits`.
Copy link
Member

Choose a reason for hiding this comment

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

I think the convention would be to write go.opentelemetry.io/otel/sdk/trace instead of otel/sdk/trace.

@@ -35,6 +38,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Removed the `WithMaxAttempts` and `WithBackoff` options from the `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp` package.
The retry logic of the package has been updated to match the `otlptracegrpc` package and accordingly a `WithRetry` option is added that should be used instead. (#2095)
- Removed metrics test package `go.opentelemetry.io/otel/sdk/export/metric/metrictest`. (#2105)
- Removed `DroppedAttributeCount` field from `otel/trace.Link` struct. (#2118)
Copy link
Member

Choose a reason for hiding this comment

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

I think the convention would be to write "go.opentelemetry.io/otel/trace".Link instead of otel/trace.Link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I just saw this and previous comment, but I guess the PR got merged to include it into v1.0.0-RC2 released today.

If needed, I can normalize these to fit the convention in a separate PR, just let me know!

@Aneurysm9 Aneurysm9 merged commit ece1879 into open-telemetry:main Jul 26, 2021
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.

Implementation details leaked into API
4 participants