Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Define some classes internal #396

Closed
pavolloffay opened this issue Apr 13, 2018 · 3 comments · Fixed by #470
Closed

Define some classes internal #396

pavolloffay opened this issue Apr 13, 2018 · 3 comments · Fixed by #470
Assignees
Milestone

Comments

@pavolloffay
Copy link
Member

See #253 (comment)

Some classes should be moved to "internal" package to avoid public use.

@pavolloffay pavolloffay added this to the 1.0 release milestone Apr 13, 2018
@yurishkuro
Copy link
Member

Looks like a meta issue. Do we have an inventory of what should be moved?

@jpkrohling
Copy link
Collaborator

jpkrohling commented Apr 13, 2018

Making this inventory would be part of the task. In general, I would say that anything non-core should be marked as "private". I would also take a white-listing approach for things in the core: place everything under "internal", and then move them out based on their expected consumption:

  • io.jaegertracing -- things meant to be used by applications directly, like the Configuration object
  • io.jaegertracing.spi -- service provider interfaces, like senders, reporters, ...
  • io.jaegertracing.internal -- for things that are meant to be consumed only by our own components, like utility classes

@jpkrohling jpkrohling self-assigned this May 15, 2018
jpkrohling added a commit to jpkrohling/jaeger-client-java that referenced this issue May 28, 2018
Resolves jaegertracing#396

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling added a commit to jpkrohling/jaeger-client-java that referenced this issue May 28, 2018
Resolves jaegertracing#396

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling added a commit to jpkrohling/jaeger-client-java that referenced this issue May 28, 2018
Resolves jaegertracing#396

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@ghost ghost added the review label May 28, 2018
@mabn
Copy link
Contributor

mabn commented Jun 19, 2018

When I wrote wrappers for jaeger there was one case where I had to use a package-protected method: SpanContext.isDebugIdContainerOnly().

Basically when passing wrapped SpanContext to a SpanBuilder I needed to be able to tell if it contains proper IDs or not.

From my point of view an ideal solution would be some better way to pass extra information from Extractor to SpanBuilder.start(). Right now the only ones are:

  • put something temporarily into the baggage
  • or set debugId flag (which cannot be read)

One more issue I had is that my traceId/spanId are strings and converting them to long looses information, so I had to put my IDs into the baggage for a while anyway - to store them for later propagation to other services.

Maybe it would make sense to create ExtractedSpanContext - a subtype of SpanContext which contains a map with extra properties?

I could avoid all these by not implementing Extractor interface, never delegating to jaeger's extract and instead writing my own extraction - it would be actually much simpler because then I could use my own SpanContext type.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants