Skip to content

Commit 7df6975

Browse files
authored
Simplify Bundling of UI assets (#5917)
## Which problem is this PR solving? - Closes #5913 ## Description of the changes - Created one file `assets.go` that holds both the `placeholder` and `actual` assets. - Added a helper `GetStaticFiles` to get the assets based on whether the actual filesystem has an `index.html.gz`. If it does, we return the assets. Otherwise, we return the placeholder `index.html` file. ## How was this change tested? Added unit tests and performed the following manual tests ### Test 1 Makefile Target ```Makefile .PHONY: run-all-in-one run-all-in-one: go run ./cmd/all-in-one --log-level debug ``` Clearing out the `actual` directory and ran `make run-all-in-one`. Going to `http://localhost:16686/` renders the placeholder HTML page. ### Test 2 Makefile Target ```Makefile .PHONY: run-all-in-one run-all-in-one: build-ui go run ./cmd/all-in-one --log-level debug ``` Ran `make run-all-in-one`. Going to `http://localhost:16686/` renders the Jaeger UI. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Mahad Zaryab <[email protected]>
1 parent 5bd6980 commit 7df6975

File tree

14 files changed

+121
-63
lines changed

14 files changed

+121
-63
lines changed

CONTRIBUTING.md

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,6 @@ the source code for the UI assets (requires Node.js 6+).
7676
The assets must be compiled first with `make build-ui`, which runs Node.js build and then
7777
packages the assets into a Go file that is `.gitignore`-ed.
7878

79-
The packaged assets can be enabled by providing a build tag `ui`, for example:
80-
81-
```
82-
$ go run -tags ui ./cmd/all-in-one/main.go
83-
```
84-
8579
`make run-all-in-one` essentially runs Jaeger all-in-one by combining both of the above
8680
steps into a single `make` command.
8781

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ lint-go: $(LINT)
186186

187187
.PHONY: run-all-in-one
188188
run-all-in-one: build-ui
189-
go run -tags ui ./cmd/all-in-one --log-level debug
189+
go run ./cmd/all-in-one --log-level debug
190190

191191
.PHONY: changelog
192192
changelog:

Makefile.BuildBinaries.mk

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,11 @@ _build-a-binary-%:
7474

7575
.PHONY: build-jaeger
7676
build-jaeger: BIN_NAME = jaeger
77-
build-jaeger: GO_TAGS = -tags ui
7877
build-jaeger: BUILD_INFO = $(BUILD_INFO_V2)
7978
build-jaeger: build-ui _build-a-binary-jaeger$(SUFFIX)-$(GOOS)-$(GOARCH)
8079

8180
.PHONY: build-all-in-one
8281
build-all-in-one: BIN_NAME = all-in-one
83-
build-all-in-one: GO_TAGS = -tags ui
8482
build-all-in-one: build-ui _build-a-binary-all-in-one$(SUFFIX)-$(GOOS)-$(GOARCH)
8583

8684
.PHONY: build-agent
@@ -89,7 +87,6 @@ build-agent: _build-a-binary-agent$(SUFFIX)-$(GOOS)-$(GOARCH)
8987

9088
.PHONY: build-query
9189
build-query: BIN_NAME = query
92-
build-query: GO_TAGS = -tags ui
9390
build-query: build-ui _build-a-binary-query$(SUFFIX)-$(GOOS)-$(GOARCH)
9491

9592
.PHONY: build-collector

cmd/query/app/static_handler.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,11 @@ type loadedConfig struct {
7676

7777
// NewStaticAssetsHandler returns a StaticAssetsHandler
7878
func NewStaticAssetsHandler(staticAssetsRoot string, options StaticAssetsHandlerOptions) (*StaticAssetsHandler, error) {
79-
assetsFS := ui.StaticFiles
79+
assetsFS := ui.GetStaticFiles(options.Logger)
8080
if staticAssetsRoot != "" {
8181
assetsFS = http.Dir(staticAssetsRoot)
8282
}
8383

84-
if options.Logger == nil {
85-
options.Logger = zap.NewNop()
86-
}
87-
8884
h := &StaticAssetsHandler{
8985
options: options,
9086
assetsFS: assetsFS,

cmd/query/app/static_handler_test.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@ import (
2929
//go:generate mockery -all -dir ../../../pkg/fswatcher
3030

3131
func TestNotExistingUiConfig(t *testing.T) {
32-
handler, err := NewStaticAssetsHandler("/foo/bar", StaticAssetsHandlerOptions{})
32+
handler, err := NewStaticAssetsHandler("/foo/bar", StaticAssetsHandlerOptions{
33+
Logger: zap.NewNop(),
34+
})
3335
require.Error(t, err)
3436
assert.Contains(t, err.Error(), "no such file or directory")
3537
assert.Nil(t, handler)
@@ -155,11 +157,18 @@ func TestRegisterStaticHandler(t *testing.T) {
155157
}
156158

157159
func TestNewStaticAssetsHandlerErrors(t *testing.T) {
158-
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/invalid-config"})
160+
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{
161+
UIConfigPath: "fixture/invalid-config",
162+
Logger: zap.NewNop(),
163+
})
159164
require.Error(t, err)
160165

161166
for _, base := range []string{"x", "x/", "/x/"} {
162-
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/ui-config.json", BasePath: base})
167+
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{
168+
UIConfigPath: "fixture/ui-config.json",
169+
BasePath: base,
170+
Logger: zap.NewNop(),
171+
})
163172
require.Errorf(t, err, "basePath=%s", base)
164173
assert.Contains(t, err.Error(), "invalid base path")
165174
}

cmd/query/app/ui/actual.go

Lines changed: 0 additions & 21 deletions
This file was deleted.

cmd/query/app/ui/assets.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// Copyright (c) 2024 The Jaeger Authors.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package ui
5+
6+
import (
7+
"embed"
8+
"net/http"
9+
10+
"go.uber.org/zap"
11+
12+
"github.com/jaegertracing/jaeger/pkg/gzipfs"
13+
"github.com/jaegertracing/jaeger/pkg/httpfs"
14+
)
15+
16+
//go:embed actual/*
17+
var actualAssetsFS embed.FS
18+
19+
//go:embed placeholder/index.html
20+
var placeholderAssetsFS embed.FS
21+
22+
// GetStaticFiles gets the static assets that the Jaeger UI will serve. If the actual
23+
// assets are available, then this function will return them. Otherwise, a
24+
// non-functional index.html is returned to be used as a placeholder.
25+
func GetStaticFiles(logger *zap.Logger) http.FileSystem {
26+
if _, err := actualAssetsFS.ReadFile("actual/index.html.gz"); err != nil {
27+
logger.Warn("ui assets not embedded in the binary, using a placeholder", zap.Error(err))
28+
return httpfs.PrefixedFS("placeholder", http.FS(placeholderAssetsFS))
29+
}
30+
31+
return httpfs.PrefixedFS("actual", http.FS(gzipfs.New(actualAssetsFS)))
32+
}

cmd/query/app/ui/assets_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Copyright (c) 2024 The Jaeger Authors.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package ui
5+
6+
import (
7+
"embed"
8+
"io"
9+
"testing"
10+
11+
"github.com/stretchr/testify/require"
12+
13+
"github.com/jaegertracing/jaeger/cmd/query/app/ui/testdata"
14+
"github.com/jaegertracing/jaeger/pkg/testutils"
15+
)
16+
17+
func TestGetStaticFiles_ReturnsPlaceholderWhenActualNotPresent(t *testing.T) {
18+
// swap out the assets FS for an empty one and then replace it back when the
19+
// test completes
20+
currentActualAssets := actualAssetsFS
21+
actualAssetsFS = embed.FS{}
22+
t.Cleanup(func() { actualAssetsFS = currentActualAssets })
23+
24+
logger, logBuf := testutils.NewLogger()
25+
fs := GetStaticFiles(logger)
26+
file, err := fs.Open("/index.html")
27+
require.NoError(t, err)
28+
bytes, err := io.ReadAll(file)
29+
require.NoError(t, err)
30+
require.Contains(t, string(bytes), "This is a placeholder for the Jaeger UI home page")
31+
require.Contains(t, logBuf.String(), "ui assets not embedded in the binary, using a placeholder")
32+
}
33+
34+
func TestGetStaticFiles_ReturnsActualWhenPresent(t *testing.T) {
35+
// swap out the assets FS for a dummy one and then replace it back when the
36+
// test completes
37+
currentActualAssets := actualAssetsFS
38+
actualAssetsFS = testdata.TestFS
39+
t.Cleanup(func() { actualAssetsFS = currentActualAssets })
40+
41+
logger, logBuf := testutils.NewLogger()
42+
fs := GetStaticFiles(logger)
43+
file, err := fs.Open("/index.html")
44+
require.NoError(t, err)
45+
bytes, err := io.ReadAll(file)
46+
require.NoError(t, err)
47+
require.NotContains(t, string(bytes), "This is a placeholder for the Jaeger UI home page")
48+
require.NotContains(t, logBuf.String(), "ui assets not embedded in the binary, using a placeholder")
49+
}

cmd/query/app/ui/doc.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
// Package ui bundles UI assets packaged with stdlib/embed.
5-
// By default it imports the placeholder, non-functional index.html.
6-
// When building with "-tags ui", it imports the real UI assets
7-
// generated in build-ui Makefile target.
5+
// If the real UI assets are available, they are embedded and
6+
// used at runtime. Otherwise, the non-functional index.html is used.
87
package ui

cmd/query/app/ui/placeholder.go

Lines changed: 0 additions & 20 deletions
This file was deleted.

0 commit comments

Comments
 (0)