Skip to content

Allow subclassing SpanContext by other packages#1940

Closed
tylerbenson wants to merge 1 commit intoopen-telemetry:masterfrom
tylerbenson:tyler/spancontext-abstract
Closed

Allow subclassing SpanContext by other packages#1940
tylerbenson wants to merge 1 commit intoopen-telemetry:masterfrom
tylerbenson:tyler/spancontext-abstract

Conversation

@tylerbenson
Copy link
Copy Markdown
Member

Having abstract methods with default visibility makes the class not able to be subclassed by other packages.
Changing to protected to maintain the intended restrictions from #1594, but allowing it to be subclassed.

Having abstract methods with default visibility makes the class not able to be subclassed by other packages.
Changing to protected to maintain the intended restrictions from open-telemetry#1594, but allowing it to be subclassed.
@tylerbenson tylerbenson requested a review from jkwatson October 30, 2020 14:20
@tylerbenson tylerbenson changed the title Allow subclassing by other packages Allow subclassing SpanContext by other packages Oct 30, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 30, 2020

Codecov Report

Merging #1940 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1940      +/-   ##
============================================
+ Coverage     86.63%   86.67%   +0.04%     
- Complexity     1501     1502       +1     
============================================
  Files           184      184              
  Lines          5662     5664       +2     
  Branches        582      582              
============================================
+ Hits           4905     4909       +4     
+ Misses          551      550       -1     
+ Partials        206      205       -1     
Impacted Files Coverage Δ Complexity Δ
.../api/baggage/propagation/W3CBaggagePropagator.java 93.18% <ø> (ø) 15.00 <0.00> (ø)
...ava/io/opentelemetry/api/trace/PropagatedSpan.java 79.16% <ø> (ø) 17.00 <0.00> (ø)
...n/java/io/opentelemetry/api/trace/SpanContext.java 89.47% <ø> (ø) 14.00 <0.00> (ø)
.../java/io/opentelemetry/context/DefaultContext.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...ain/java/io/opentelemetry/context/LazyStorage.java 88.46% <ø> (ø) 9.00 <0.00> (ø)
...va/io/opentelemetry/opentracingshim/TraceShim.java 100.00% <100.00%> (ø) 4.00 <1.00> (+1.00)
...y/sdk/metrics/aggregator/DoubleMinMaxSumCount.java 100.00% <0.00%> (+4.08%) 6.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6f111a...89d1ca3. Read the comment docs.

@jkwatson
Copy link
Copy Markdown
Contributor

Why do you need to create a non-AutoValue subclass of SpanContext? I think this is probably not a good idea, since we do not want the SpanContext to end up mutable. If we allow subclassing, we cannot guarantee that.

Is there another way to solve your use-case that doesn't open up the possibility of breaking things?

@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Oct 30, 2020

@tylerbenson The spec says that the SpanContext functionality SHOULD NOT be overridable, so I don't think we should do that.

@tylerbenson
Copy link
Copy Markdown
Member Author

Closing in favor of #1935.

@tylerbenson tylerbenson deleted the tyler/spancontext-abstract branch October 30, 2020 16:39
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.

3 participants