Define some classes internal#470
Conversation
ghost
assigned
Codecov Report
@@ Coverage Diff @@
## master #470 +/- ##
============================================
- Coverage 88.12% 88.09% -0.03%
+ Complexity 493 489 -4
============================================
Files 63 62 -1
Lines 1852 1848 -4
Branches 241 241
============================================
- Hits 1632 1628 -4
Misses 142 142
Partials 78 78
Continue to review full report at Codecov.
|
|
@objectiser I believe this PR follows what was discussed last Friday during the bi-weekly meeting. Would you mind reviewing it ? |
| package io.jaegertracing.baggage; | ||
| package io.jaegertracing.spi; | ||
|
|
||
| import io.jaegertracing.internal.baggage.Restriction; |
There was a problem hiding this comment.
What should we do in these cases - the Restriction class is part of the SPI - we shouldn't really be returning or passing instances of internal classes in a public API?
There was a problem hiding this comment.
Last Friday we talked briefly about this: providers (those who will consume the SPI) are expected to depend on internal classes, like JaegerSpan (we discussed it when talking about the Reporters and Senders).
End consumes of our code will only ever be required to deal with the Configuration object and OpenTracing API, at least for this initial version. If we later decide that some class should really be part of the public API, we'll white list that specific one.
There was a problem hiding this comment.
Yeah agree that was what we discussed - but on reflection I'm thinking that if a class is part of the "public" API/SPI, then it should also be public - as end users may contribute SPI implementations.
I'll have a look through the current SPI interfaces to see how many current internal classes are affected - if not many, it may be worth moving them to be public again.
There was a problem hiding this comment.
as end users may contribute SPI implementations.
Then they are wearing the "provider" hat on this code/module. Their "consumer" code shouldn't make use of the Restriction class, only their provider implementation. And provider implementations are expected to deal with internal classes.
It's still not my first choice, as I'd rather have clear compatibility expectations not only for end consumers (public API) but also for providers: as a provider, can I rely on the getTracer() method from JaegerSpan?
But, the current arrangement is already better than the status quo :)
There was a problem hiding this comment.
Not sure I agree with providers having to deal with internal classes. If the provider is defined independent of the jaeger code base, it should expect a stable SPI, which is not guaranteed in relation to the internal classes.
+1 I would prefer JaegerSpan was a public interface, with internal JaegerSpanImpl - even if for now JaegerSpan had no other methods other than derived from Span.
There was a problem hiding this comment.
it should expect a stable SPI, which is not guaranteed in relation to the internal classes.
Agreed.
+1 I would prefer JaegerSpan was a public interface, with internal JaegerSpanImpl - even if for now JaegerSpan had no other methods other than derived from Span.
The other suggestion made during the call was to indeed have this split, but with empty interfaces, like public interface JaegerSpan extends Span {}.
|
@jpkrohling If In terms of the internal implementations of (for example) samplers - are they going to be made public based on request? Otherwise not sure the tracer builder approach is going to be useful, if the app needs to then use internal sampler implementations? |
That's my understanding, yes. The idea is that our implementations would be assigned based on configuration options like it is currently being done with |
|
I am not seeing a lot of requests from people to provide a stable SPI. In the absence of said requests I would be comfortable saying that we simply do not have a stable SPI at this point, and our semver guarantees only apply to public APIs. |
jpkrohling
force-pushed
the
396-Define-classes-as-internal
branch
from
27f348b to
96ad3b0
Compare
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
it would be really good to have some eyes on this from people who actually extend the client.
jaeger-core/README.md
Outdated
| ### Production | ||
|
|
||
| Tracer can be created via `io.jaegertracing.JaegerTracer.Builder` or `io.jaegertracing.Configuration`. | ||
| Tracer can be created via `io.jaegertracing.internal.JaegerTracer.Builder` or `io.jaegertracing.Configuration`. |
There was a problem hiding this comment.
mention Configuration first, and add (preferred).
| import io.opentracing.Tracer; | ||
| import io.jaegertracing.internal.JaegerTracer; | ||
| import io.jaegertracing.internal.metrics.Metrics; | ||
| import io.jaegertracing.internal.metrics.MetricsFactory; |
There was a problem hiding this comment.
MetricsFactory and NoopMetricsFactory are part of SPI
There was a problem hiding this comment.
I actually left the MetricsFactory out of the SPI on purpose, as I thought this was too internal to be customized. Given that we already delegate to an abstraction (Micrometer), it means people can just build a Micrometer reporter instead.
About NoopMetricsFactory, I don't think it belongs to the SPI. Would you mind sharing why you think it would?
There was a problem hiding this comment.
You're probably right about noop factory, it's just used as a default. But I think MetricsFactory belongs to an SPI because we don't know upfront which metrics API people want to use with the client. Does Micrometer really support everything?
There was a problem hiding this comment.
It's an abstraction with tons of providers already implemented: http://micrometer.io/docs
If a provider is not there, people would either have to implement a micrometer provider or a MetricsFactory provider, so, pretty much the same work (but their Micrometer might be reused by other applications and in other scenarios).
I'd really encourage our users to use Micrometer with their own provider than implementing their own MetricsFactory provider.
There was a problem hiding this comment.
That's assuming people use oss frameworks. For example, we internally have a Java client for Uber's m3db metrics system. Rather than implementing an adapter through Micrometer, it makes more sense for us to bind directly to jaeger's MetricsFactory (which we already do).
There was a problem hiding this comment.
That's assuming people use oss frameworks
In general, like, Jaeger? :-) Or do you mean for the metrics backend, like Prometheus?
Rather than implementing an adapter through Micrometer, it makes more sense for us to bind directly to jaeger's MetricsFactory (which we already do).
I would argue that this is like using the (internal) JaegerTracer.Builder: there are valid use cases, but it's not the usual case. The usual case is for an application to be using a framework like Micrometer to make data available to a backend like Prometheus.
How about we keep this internal first and, if there's demand, we make it part of the SPI?
There was a problem hiding this comment.
In general, like, Jaeger? :-) Or do you mean for the metrics backend, like Prometheus?
I mean oss frameworks for reporting metrics. Organizations adopting Jaeger may be running all kinds of custom metrics backends, respectively with custom libraries for sending metrics to those backends, which is the case at Uber, as I don't think our metrics client for Java is even open source.
I am not suggesting making MetricsFactory a public interface, but moving it to spi. You already have a bunch of interfaces there, which in my view are actually a lot less likely to be implemented by other people, e.g. who would need to implement Sampler or Sender? If people do implement those, they are doing something very tracing-specific, whereas implement MetricsFactory is simply adapting Jaeger lib to run in a custom enterprise environment.
|
I think TracerBuilder should belong to the public API with all dependent classes e.g. samplers, reporters and so on. Are we assuming nobody is extending Jaeger tracer, sampler, sender? |
We have decided last week to not go this route.
I don't think we are assuming that. At this point, we are not aware of people doing that and we'll open up the API based on the demand. The most important right now is signal to end consumers that Jaeger APIs aren't public API, other than the Sampler/Sender are part of the SPI, and we decided last week that it's OK for providers to have access to internal classes, like they do today. If providers need better stability in the classes that they consume (like, JaegerSpan), then we might make them part of a public API in the future. |
|
Only one of the links you provided is for a usage of We are certainly going to break current clients, but with that, we hope to provide a better expectation around backwards compatibility. Edit: I just noticed you said builders, not tracer builder. But the main message remains. |
There are way more references to tracer builder, I wanted to pick others too like extending a reporter. |
| import io.opentracing.Tracer; | ||
| import io.jaegertracing.internal.JaegerTracer; | ||
| import io.jaegertracing.internal.metrics.Metrics; | ||
| import io.jaegertracing.internal.metrics.MetricsFactory; |
There was a problem hiding this comment.
In general, like, Jaeger? :-) Or do you mean for the metrics backend, like Prometheus?
I mean oss frameworks for reporting metrics. Organizations adopting Jaeger may be running all kinds of custom metrics backends, respectively with custom libraries for sending metrics to those backends, which is the case at Uber, as I don't think our metrics client for Java is even open source.
I am not suggesting making MetricsFactory a public interface, but moving it to spi. You already have a bunch of interfaces there, which in my view are actually a lot less likely to be implemented by other people, e.g. who would need to implement Sampler or Sender? If people do implement those, they are doing something very tracing-specific, whereas implement MetricsFactory is simply adapting Jaeger lib to run in a custom enterprise environment.
| */ | ||
|
|
||
| package io.jaegertracing; | ||
| package io.jaegertracing.internal; |
There was a problem hiding this comment.
some of these technically could be considered public, e.g.
- TRACER_IP_TAG_KEY - there have been requests to allow providing IP manually via tracer tag.
- SAMPLER_TYPE_TAG_KEY, SAMPLER_PARAM_TAG_KEY - analytics jobs often need to read and interpret these span tags
- DEBUG_ID_HEADER_KEY - feels like public, but not sure what the exact use case is.
There was a problem hiding this comment.
I'll open a new follow-up PR with a new constants class, to discuss and move the candidates.
| */ | ||
|
|
||
| package io.jaegertracing.utils; | ||
| package io.jaegertracing.internal.utils; |
Moved everything into internal packages, except for a few selected classes. Most of the interfaces were moved to the SPI package. Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
396-Define-classes-as-internal
branch
from
113022e to
dfb9d8b
Compare
|
Classes in package "io.jaegertracing.internal.clock" declare wrong package name, such as: |
PR jaegertracing#470 brings inconsistent package name. Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
PR jaegertracing#470 brings inconsistent package name. Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
PR #470 brings inconsistent package name. Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
5 participants
Which problem is this PR solving?
Short description of the changes
internalpackage, except for:ConfigurationobjectZipkinsender and reporterMicrometerMetricsFactoryDecide
JaegerTracer.Builder. It should really be public, but it belongs to theJaegerTracerclass, which is internal. Should this builder be moved out of the class, or should we make this builderinternal? My preference would be to extract it toio.jaegertracing.TracerBuilderclass.