-
Notifications
You must be signed in to change notification settings - Fork 834
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
De-singleton ZPageServer implementation #4935
Merged
jack-berg
merged 5 commits into
open-telemetry:main
from
breedx-splk:de-singleton_zpages
Nov 12, 2022
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b1efc40
remove the static/singleton nature from ZPageServer
breedx-splk 48af61d
add convenience method for creating zpages-based sdktracerprovider
breedx-splk 18609c8
update readme for zpageserver
breedx-splk 84e6a4f
oh checkstyle.
breedx-splk 4ecb895
overload build and add test
breedx-splk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ | |
|
||
import com.sun.net.httpserver.HttpServer; | ||
import io.opentelemetry.api.internal.GuardedBy; | ||
import io.opentelemetry.sdk.trace.SdkTracerProvider; | ||
import io.opentelemetry.sdk.trace.SdkTracerProviderBuilder; | ||
import io.opentelemetry.sdk.trace.SpanLimits; | ||
import io.opentelemetry.sdk.trace.SpanProcessor; | ||
import io.opentelemetry.sdk.trace.samplers.Sampler; | ||
|
@@ -74,42 +76,46 @@ public final class ZPageServer { | |
// Length of time to wait for the HttpServer to stop | ||
private static final int HTTPSERVER_STOP_DELAY = 1; | ||
// Tracez SpanProcessor and DataAggregator for constructing TracezZPageHandler | ||
private static final TracezSpanProcessor tracezSpanProcessor = | ||
TracezSpanProcessor.builder().build(); | ||
private static final TracezTraceConfigSupplier tracezTraceConfigSupplier = | ||
private final TracezSpanProcessor tracezSpanProcessor = TracezSpanProcessor.builder().build(); | ||
private final TracezTraceConfigSupplier tracezTraceConfigSupplier = | ||
new TracezTraceConfigSupplier(); | ||
private static final TracezDataAggregator tracezDataAggregator = | ||
private final TracezDataAggregator tracezDataAggregator = | ||
new TracezDataAggregator(tracezSpanProcessor); | ||
// Handler for /tracez page | ||
private static final ZPageHandler tracezZPageHandler = | ||
new TracezZPageHandler(tracezDataAggregator); | ||
private final ZPageHandler tracezZPageHandler = new TracezZPageHandler(tracezDataAggregator); | ||
// Handler for /traceconfigz page | ||
private static final ZPageHandler traceConfigzZPageHandler = | ||
private final ZPageHandler traceConfigzZPageHandler = | ||
new TraceConfigzZPageHandler(tracezTraceConfigSupplier); | ||
// Handler for index page, **please include all available ZPageHandlers in the constructor** | ||
private static final ZPageHandler indexZPageHandler = | ||
private final ZPageHandler indexZPageHandler = | ||
new IndexZPageHandler(Arrays.asList(tracezZPageHandler, traceConfigzZPageHandler)); | ||
|
||
private static final Object mutex = new Object(); | ||
private final Object mutex = new Object(); | ||
|
||
@GuardedBy("mutex") | ||
@Nullable | ||
private static HttpServer server; | ||
private HttpServer server; | ||
|
||
private ZPageServer() {} | ||
|
||
public static ZPageServer create() { | ||
return new ZPageServer(); | ||
} | ||
|
||
/** Returns a supplier of {@link SpanLimits} which can be reconfigured using zpages. */ | ||
public static Supplier<SpanLimits> getTracezTraceConfigSupplier() { | ||
public Supplier<SpanLimits> getTracezTraceConfigSupplier() { | ||
return tracezTraceConfigSupplier; | ||
} | ||
|
||
/** Returns a {@link Sampler} which can be reconfigured using zpages. */ | ||
public static Sampler getTracezSampler() { | ||
public Sampler getTracezSampler() { | ||
return tracezTraceConfigSupplier; | ||
} | ||
|
||
/** | ||
* Returns a {@link SpanProcessor} which will allow processing of spans by {@link ZPageServer}. | ||
*/ | ||
public static SpanProcessor getSpanProcessor() { | ||
public SpanProcessor getSpanProcessor() { | ||
return tracezSpanProcessor; | ||
} | ||
|
||
|
@@ -119,7 +125,7 @@ public static SpanProcessor getSpanProcessor() { | |
* | ||
* @param server the {@link HttpServer} for the page to register to. | ||
*/ | ||
static void registerIndexZPageHandler(HttpServer server) { | ||
private void registerIndexZPageHandler(HttpServer server) { | ||
server.createContext(indexZPageHandler.getUrlPath(), new ZPageHttpHandler(indexZPageHandler)); | ||
} | ||
|
||
|
@@ -138,7 +144,7 @@ static void registerIndexZPageHandler(HttpServer server) { | |
* | ||
* @param server the {@link HttpServer} for the page to register to. | ||
*/ | ||
static void registerTracezZPageHandler(HttpServer server) { | ||
private void registerTracezZPageHandler(HttpServer server) { | ||
server.createContext(tracezZPageHandler.getUrlPath(), new ZPageHttpHandler(tracezZPageHandler)); | ||
} | ||
|
||
|
@@ -154,7 +160,7 @@ static void registerTracezZPageHandler(HttpServer server) { | |
* | ||
* @param server the {@link HttpServer} for the page to register to. | ||
*/ | ||
static void registerTraceConfigzZPageHandler(HttpServer server) { | ||
private void registerTraceConfigzZPageHandler(HttpServer server) { | ||
server.createContext( | ||
traceConfigzZPageHandler.getUrlPath(), new ZPageHttpHandler(traceConfigzZPageHandler)); | ||
} | ||
|
@@ -164,7 +170,7 @@ static void registerTraceConfigzZPageHandler(HttpServer server) { | |
* | ||
* @param server the {@link HttpServer} for the page to register to. | ||
*/ | ||
public static void registerAllPagesToHttpServer(HttpServer server) { | ||
public void registerAllPagesToHttpServer(HttpServer server) { | ||
// For future zPages, register them to the server in here | ||
registerIndexZPageHandler(server); | ||
registerTracezZPageHandler(server); | ||
|
@@ -173,7 +179,7 @@ public static void registerAllPagesToHttpServer(HttpServer server) { | |
} | ||
|
||
/** Method for stopping the {@link HttpServer} {@code server}. */ | ||
private static void stop() { | ||
private void stop() { | ||
synchronized (mutex) { | ||
if (server == null) { | ||
return; | ||
|
@@ -183,6 +189,30 @@ private static void stop() { | |
} | ||
} | ||
|
||
/** | ||
* Convenience method to return a new SdkTracerProvider that has been configured with our ZPage | ||
* specific span processor, sampler, and limits. | ||
* | ||
* @return new SdkTracerProvider | ||
*/ | ||
public SdkTracerProvider buildSdkTracerProvider() { | ||
return buildSdkTracerProvider(SdkTracerProvider.builder()); | ||
} | ||
|
||
/** | ||
* Convenience method to return a new SdkTracerProvider that has been configured with our ZPage | ||
* specific span processor, sampler, and limits. | ||
* | ||
* @return new SdkTracerProvider | ||
*/ | ||
public SdkTracerProvider buildSdkTracerProvider(SdkTracerProviderBuilder builder) { | ||
return builder | ||
.addSpanProcessor(getSpanProcessor()) | ||
.setSpanLimits(getTracezTraceConfigSupplier()) | ||
.setSampler(getTracezSampler()) | ||
.build(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have this accept a Also, can we add test coverage for this method? |
||
|
||
/** | ||
* Starts a private {@link HttpServer} and registers all zPages to it. When the JVM shuts down the | ||
* server is stopped. | ||
|
@@ -193,13 +223,13 @@ private static void stop() { | |
* @throws IllegalStateException if the server is already started. | ||
* @throws IOException if the server cannot bind to the specified port. | ||
*/ | ||
public static void startHttpServerAndRegisterAllPages(int port) throws IOException { | ||
public void startHttpServerAndRegisterAllPages(int port) throws IOException { | ||
synchronized (mutex) { | ||
if (server != null) { | ||
throw new IllegalStateException("The HttpServer is already started."); | ||
} | ||
server = HttpServer.create(new InetSocketAddress(port), HTTPSERVER_BACKLOG); | ||
ZPageServer.registerAllPagesToHttpServer(server); | ||
registerAllPagesToHttpServer(server); | ||
server.start(); | ||
} | ||
|
||
|
@@ -208,10 +238,8 @@ public static void startHttpServerAndRegisterAllPages(int port) throws IOExcepti | |
new Thread() { | ||
@Override | ||
public void run() { | ||
ZPageServer.stop(); | ||
ZPageServer.this.stop(); | ||
} | ||
}); | ||
} | ||
|
||
private ZPageServer() {} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have this return void, but don't feel strongly since its a convenience method and is experimental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Yeah I totally see where you're coming from, but pure side-effect methods also give me the creeps. :)