Added support for hot reload of UI config #1688

jpkrohling · 2019-07-25T09:24:14Z

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Which problem is this PR solving?

As seen on Update of spec.ui has no effect on jaegerUI jaeger-operator#533, changing the UI configuration requires the Jaeger Query to be stopped and started again. As the file is sent as is inside the HTML that is requested by the browser, it's safe to simply watch the file for changes and replace the internal indexHTML property.

codecov · 2019-07-25T14:31:49Z

Codecov Report

Merging #1688 into master will decrease coverage by 0.15%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
- Coverage   98.51%   98.36%   -0.16%     
==========================================
  Files         193      193              
  Lines        9314     9358      +44     
==========================================
+ Hits         9176     9205      +29     
- Misses        110      119       +9     
- Partials       28       34       +6

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	`86.84% <70.58%> (-13.16%)`	⬇️

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 aa76cf0...7cf6945. Read the comment docs.

jpkrohling · 2019-07-25T14:34:21Z

The code coverage is lower than the usual because of the logging of errors related to file system problems, which are hard to reproduce in unit tests.

yurishkuro

there was another ticket to support watching of the sampling strategies file. I am not sure if there's code that we can reuse though.

cmd/query/app/static_handler.go

yurishkuro · 2019-07-25T20:33:50Z

cmd/query/app/static_handler.go

+	}
+	defer watcher.Close()
+
+	done := make(chan bool)


I am not sure why this is needed, there's no external signaling into this chan

That was mainly to keep the function from returning, as it would then call watcher.Close(), but I figured I could just not close the watcher and it would have the same effect.

cmd/query/app/static_handler.go

cmd/query/app/static_handler_test.go

jpkrohling

PR updated

jpkrohling · 2019-07-26T10:11:09Z

cmd/query/app/static_handler.go

+	}
+	defer watcher.Close()
+
+	done := make(chan bool)


That was mainly to keep the function from returning, as it would then call watcher.Close(), but I figured I could just not close the watcher and it would have the same effect.

cmd/query/app/static_handler.go

cmd/query/app/static_handler_test.go

jpkrohling · 2019-07-26T12:05:29Z

Marking this as WIP, as this PR alone would not solve the original problem from jaegertracing/jaeger-operator#533.

jpkrohling · 2019-08-02T08:07:11Z

cmd/query/app/static_handler_test.go

@@ -72,7 +73,6 @@ func TestRegisterStaticHandler(t *testing.T) {
 				BasePath:     testCase.basePath,
 				UIConfig:     "fixture/ui-config.json",
 			})
-			assert.Empty(t, buf.String(), "no logs during construction")


I removed this assertion, as I believe there's value in adding a log message stating that the config file specified via CLI is being watched.

yurishkuro

What is the use case for reloading UI config? I.e. what kind of things would people want to change there dynamically?

The file watching behavior would be quite useful for the sampling strategies config, I believe we even have a ticket for that.

cmd/query/app/static_handler.go

jpkrohling · 2019-08-05T09:03:02Z

What is the use case for reloading UI config? I.e. what kind of things would people want to change there dynamically?

@mwieczorek faced this on jaegertracing/jaeger-operator#533 . I don't expect this file to be changed that often, and most instances would probably never change, but for the cases where it does change, we'd have to kill the Jaeger Query component to have the config file re-read. We could watch for a SIGHUP and reload accordingly, but that wouldn't solve his problem, as there's no SIGHUP in Kubernetes :-)

The file watching behavior would be quite useful for the sampling strategies config, I believe we even have a ticket for that.

+1, I can work on that next, but I think that one involves more work than just reloading the config file and updating a single atomic field.

yurishkuro · 2019-08-05T22:42:16Z

I don't expect this file to be changed that often, and most instances would probably never change, but for the cases where it does change

Right, so that was my point, is it worth adding all this code (and future maintenance) for such a marginal use case? The UI config is not really that dynamic, I can't think of any items there that must be changed in place. Feels like a redeploy / restart is a perfectly valid solution.

mwieczorek · 2019-08-06T07:27:54Z

The UI config is not really that dynamic, I can't think of any items there that must be changed in place. Feels like a redeploy / restart is a perfectly valid solution.

I'll add my comment as I created the issue jaegertracing/jaeger-operator#533
Indeed changes in the ui-config are not often. In my case, it's related to links generated on span tags. From time to time I'd like to add new or modify existing one. All the changes are pushed by automated pipeline, so right now I have to restart pods after the change of Jaeger CR. I thought it could be handled by the operator (detect if there's change in ui-config part of CR spec and then somehow restart the pods).
@jpkrohling suggested it can be handled directly in jaeger.

Anyway: if the change introduces too much burden - I can just live with additional pipeline steps which will handle the restarts.

objectiser · 2019-08-06T07:31:43Z

@yurishkuro If a similar approach is of interest for sampling strategies, then wouldn't it be better for Jaeger to have a consistent approach to reloading configs?

yurishkuro · 2019-08-07T01:25:19Z

@objectiser it would be much better if there was a shared component, yet all of the new code here is embedded directly into the handler. But I don't mind merging it.

jpkrohling · 2019-08-07T09:01:54Z

The watcher part can be refactored when we add a similar support for the sampling strategy config file.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling · 2019-08-08T07:54:23Z

I'm merging this, with the promise that I'll refactor this code to suite multiple consumers of the logic, once I send a PR allowing a similar feature for the sampling strategies configuration.

jpkrohling requested review from black-adder, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners July 25, 2019 09:24

yurishkuro reviewed Jul 25, 2019

View reviewed changes

jpkrohling commented Jul 26, 2019

View reviewed changes

jpkrohling changed the title ~~Added support for hot reload of UI config~~ WIP - Added support for hot reload of UI config Jul 26, 2019

jpkrohling changed the title ~~WIP - Added support for hot reload of UI config~~ Added support for hot reload of UI config Aug 1, 2019

jpkrohling mentioned this pull request Aug 1, 2019

Update of spec.ui has no effect on jaegerUI jaegertracing/jaeger-operator#533

Closed

jpkrohling commented Aug 2, 2019

View reviewed changes

yurishkuro reviewed Aug 3, 2019

View reviewed changes

cmd/query/app/static_handler.go Outdated Show resolved Hide resolved

cmd/query/app/static_handler.go Outdated Show resolved Hide resolved

objectiser approved these changes Aug 7, 2019

View reviewed changes

Added support for hot reload of UI config

7cf6945

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling merged commit 98fd69a into jaegertracing:master Aug 8, 2019

yurishkuro mentioned this pull request Oct 7, 2019

collector: reload sampling strategies on file content change #1058

Closed

annanay25 mentioned this pull request Jan 28, 2020

Add support for dynamically reloading sampling strategies #2040

Closed

jpkrohling deleted the Hot-reload-UI-config branch July 28, 2021 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for hot reload of UI config #1688

Added support for hot reload of UI config #1688

jpkrohling commented Jul 25, 2019

codecov bot commented Jul 25, 2019 •

edited

Loading

jpkrohling commented Jul 25, 2019

yurishkuro left a comment

yurishkuro Jul 25, 2019

jpkrohling Jul 26, 2019

jpkrohling left a comment

jpkrohling Jul 26, 2019

jpkrohling commented Jul 26, 2019

jpkrohling Aug 2, 2019

yurishkuro left a comment

jpkrohling commented Aug 5, 2019

yurishkuro commented Aug 5, 2019

mwieczorek commented Aug 6, 2019

objectiser commented Aug 6, 2019

yurishkuro commented Aug 7, 2019

jpkrohling commented Aug 7, 2019

jpkrohling commented Aug 8, 2019

Added support for hot reload of UI config #1688

Added support for hot reload of UI config #1688

Conversation

jpkrohling commented Jul 25, 2019

Which problem is this PR solving?

codecov bot commented Jul 25, 2019 • edited Loading

Codecov Report

jpkrohling commented Jul 25, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Jul 25, 2019

Choose a reason for hiding this comment

jpkrohling Jul 26, 2019

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling Jul 26, 2019

Choose a reason for hiding this comment

jpkrohling commented Jul 26, 2019

jpkrohling Aug 2, 2019

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

jpkrohling commented Aug 5, 2019

yurishkuro commented Aug 5, 2019

mwieczorek commented Aug 6, 2019

objectiser commented Aug 6, 2019

yurishkuro commented Aug 7, 2019

jpkrohling commented Aug 7, 2019

jpkrohling commented Aug 8, 2019

codecov bot commented Jul 25, 2019 •

edited

Loading