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

Remove Binary format and related members. #891

Merged

Conversation

carlosalberto
Copy link
Contributor

As part of 0.3.0 we will be removing Binary (and later, under 0.5.0, putting it back once the format gets updated).

This is part of #720

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@9aa6d1e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #891   +/-   ##
=========================================
  Coverage          ?   82.82%           
  Complexity        ?      830           
=========================================
  Files             ?      110           
  Lines             ?     2870           
  Branches          ?      249           
=========================================
  Hits              ?     2377           
  Misses            ?      382           
  Partials          ?      111
Impacted Files Coverage Δ Complexity Δ
...a/io/opentelemetry/opentracingshim/TracerShim.java 62.96% <ø> (ø) 11 <0> (?)
.../io/opentelemetry/opentracingshim/Propagation.java 100% <ø> (ø) 5 <0> (?)
...ationcontext/DefaultCorrelationContextManager.java 69.23% <ø> (ø) 7 <0> (?)
...rrelationcontext/CorrelationContextManagerSdk.java 100% <ø> (ø) 5 <0> (?)
...ain/java/io/opentelemetry/trace/DefaultTracer.java 95.23% <ø> (ø) 6 <0> (?)
...ain/java/io/opentelemetry/sdk/trace/TracerSdk.java 100% <ø> (ø) 8 <0> (?)

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 9aa6d1e...4873ed4. Read the comment docs.

@Oberon00
Copy link
Member

Would it be easier to leave BinaryContext in a non-working form here? If we temporarily remove it, we break users now and later they have to put it back. If we don't remove it, we break them later and they can change their usage to the new interface right away.

@carlosalberto
Copy link
Contributor Author

I'm not sure we will be breaking users, because I don't think anybody has been using this (for example, we don't have binary support for CorrelationContext and the one for tracing is rather experimental).

In JS they decided to keep it because there were pieces already using it, so it makes sense for them to keep things around, but that's not our case. I can keep them around, but it feels weird to me to keep around 'dead-for-now' code ;)

@Oberon00
Copy link
Member

We plan to use the BinaryFormat interface at Dynatrace, with our own implementation. But if you think no one else is using it, we don't really require having that interface in the API.

@carlosalberto
Copy link
Contributor Author

I don't think anybody else than you guys is using it at this time. I imagine users relying on that once we have a full implementation though (not a partial one like the one we have right now).

I'm adding then the "Holding On" label, in case we don't move fast enough on the OTEP 66 changes ;)

@carlosalberto
Copy link
Contributor Author

Ooops, was expected to put the label to #914

@bogdandrutu Please provide your review/opinion on this one ;)

@bogdandrutu
Copy link
Member

Please rebase, also I am not sure what was the decision.

@carlosalberto
Copy link
Contributor Author

Rebased and updated. The decision from the last SIG call was to temporary remove it.

@open-telemetry/java-approvers please review & approve.

@bogdandrutu bogdandrutu merged commit 26a4070 into open-telemetry:master Mar 3, 2020
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.

5 participants