Skip to content

Conversation

@kkoz
Copy link
Member

@kkoz kkoz commented Dec 16, 2019

No description provided.

@kkoz kkoz requested a review from chris-allan December 16, 2019 20:26
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this PR is currently opened against 0.4.x and either need to be updated or reopened against master.
More generally, this addresses an issue that still persists in the HEAD of omero-ms-image-region

if (tracingEnabled) {
String zipkinUrl = httpTracingConfig.getString("zipkin-url");
log.info("Tracing enabled: {}", zipkinUrl);
sender = OkHttpSender.create(zipkinUrl);
spanReporter = AsyncReporter.create(sender);
PrometheusSpanHandler prometheusSpanHandler = new PrometheusSpanHandler();
tracing = Tracing.newBuilder()
.sampler(Sampler.ALWAYS_SAMPLE)
.localServiceName("omero-ms-image-region")
.addFinishedSpanHandler(prometheusSpanHandler)
.spanReporter(spanReporter)
.build();
} else {
log.info("Tracing disabled");
PrometheusSpanHandler prometheusSpanHandler = new PrometheusSpanHandler();
spanReporter = new LogSpanReporter();
tracing = Tracing.newBuilder()
.sampler(Sampler.ALWAYS_SAMPLE)
.localServiceName("omero-ms-image-region")
.addFinishedSpanHandler(prometheusSpanHandler)
.spanReporter(spanReporter)
.build();
}

Currently, depending on the http-tracing section of the micro-service configuration:

  • if enabled is true, the current behavior assumes that zipkin-url is set-up and the tracing is sent there
  • if enabled is false, the current behavior is to sent the tracing to to the log
  • the latter is currently the default behavior as per the config.yaml shipped with the micro-service

I concur with the proposal to respect the value of the enabled key to enable/disable tracing. This leaves the question of how to decide the storage backend which should be used. The current version of this PR proposes to detect some special pattern for the zipkin-url value. My primary concern with this approach is that it feels like a misuse. In all cases, this will constitute effectively a breaking change and the configuration of micro-services sending tracing to the logs will need to be reviewed and updated.

Alternate solutions I could think of:

  • detect a simpler magic value for sending the tracing to the logs e.g. an empty string
  • support the presence or absence of the zipkin-url key to decide whether to use the logs or zipkin as the storage backend i.e. commenting the key in the configuration would mean using the logs
  • define a more advanced backend key allowing full configurability. Although it is likely the most generic approach, it certainly feels like a lot of engineering and really make sense if we start introducing a third backend (as we really don't want to start relying on regex for thesese)

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.

2 participants