Skip to content
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

lens: fixes BASE_PATH handling and re-aligns to old resource paths #3722

Merged
merged 1 commit into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@
<exclude>**/.idea/**</exclude>
<exclude>**/node_modules/**</exclude>
<exclude>**/build/**</exclude>
<exclude>**/dist/**</exclude>
<exclude>**/coverage/**</exclude>
<exclude>**/.babelrc</exclude>
<exclude>**/.bowerrc</exclude>
Expand Down
2 changes: 0 additions & 2 deletions zipkin-lens/.env.development

This file was deleted.

2 changes: 0 additions & 2 deletions zipkin-lens/.env.production

This file was deleted.

2 changes: 0 additions & 2 deletions zipkin-lens/.env.test

This file was deleted.

1 change: 1 addition & 0 deletions zipkin-lens/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

# production
/build
/dist

# misc
.DS_Store
Expand Down
2 changes: 2 additions & 0 deletions zipkin-lens/.npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# This is needed while dependencies rely on different versions of react
legacy-peer-deps=true
Copy link
Member Author

Choose a reason for hiding this comment

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

unless you put this here, you have to remember not just for npm install, but also audit

12 changes: 12 additions & 0 deletions zipkin-lens/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ To get a coverage report as well, run `npm test -- --coverage`.
Builds the app for production to the `build` folder.<br />
It correctly bundles React in production mode and optimizes the build for the best performance.

## Build tips

### Use the production node version

The production UI is built with Maven. To use the same version, issue this command:

```bash
nvm use $(../mvnw help:evaluate -Dexpression=node.version -q -DforceStdout)
```

Now, it is less likely a pull request will fail when `npm test` succeeds locally.

## Localization

We use [LinguiJS](https://lingui.js.org/) for localization of the UI. Translations for strings are
Expand Down
1 change: 1 addition & 0 deletions zipkin-lens/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<!DOCTYPE html>
<html>
<head>
<base href="/zipkin" >
Copy link
Member Author

Choose a reason for hiding this comment

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

without this, anything that emulates zipkin-server's ZIPKIN_UI_BASE_PATH handling would have broke.

<meta charset="utf-8" />
<link rel="icon" href="/favicon.ico" />
</head>
Expand Down
8 changes: 4 additions & 4 deletions zipkin-lens/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion zipkin-lens/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
<goal>npm</goal>
</goals>
<configuration>
<arguments>install --legacy-peer-deps</arguments>
<arguments>install</arguments>
</configuration>
</execution>
<execution>
Expand Down
2 changes: 1 addition & 1 deletion zipkin-lens/src/components/App/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const App: React.FC = () => {
const baseName = useMemo(() => {
return import.meta.env.DEV
? '/zipkin'
: (import.meta.env.BASE_PATH as string);
: (import.meta.env.BASE_URL as string);
}, []);

return (
Expand Down
2 changes: 1 addition & 1 deletion zipkin-lens/src/components/App/LanguageSelector.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand Down
2 changes: 1 addition & 1 deletion zipkin-lens/src/components/App/Layout.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand Down
3 changes: 1 addition & 2 deletions zipkin-lens/src/components/App/TraceIdSearch.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import { TextField } from '@material-ui/core';
import React, { useCallback, useState } from 'react';
import { useHistory } from 'react-router-dom';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import {
Table,
TableBody,
Expand Down
3 changes: 1 addition & 2 deletions zipkin-lens/src/components/TracePage/Header/Header.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import { Box, Button, makeStyles, Typography } from '@material-ui/core';
import { List as ListIcon } from '@material-ui/icons';
import React from 'react';
Expand Down
3 changes: 1 addition & 2 deletions zipkin-lens/src/components/TracePage/Header/HeaderMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import { Button, makeStyles, Menu, MenuItem } from '@material-ui/core';
import { Menu as MenuIcon } from '@material-ui/icons';
import React, { useCallback } from 'react';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import { Box, Divider, Grid, makeStyles, Typography } from '@material-ui/core';
import React from 'react';
import { useTranslation } from 'react-i18next';
Expand Down
3 changes: 1 addition & 2 deletions zipkin-lens/src/components/TracePage/Timeline/Timeline.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import { Box, makeStyles } from '@material-ui/core';
import React, { useCallback, useEffect, useRef } from 'react';
import AutoSizer, { Size } from 'react-virtualized-auto-sizer';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,65 +13,60 @@
*/
import { render, screen } from '@testing-library/react';
import fetchMock from 'fetch-mock';
import { afterEach, beforeEach, it, describe, expect } from 'vitest';
import { afterEach, it, describe, expect, vi } from 'vitest';
import React, { Suspense } from 'react';

import {
defaultConfig,
UiConfigConsumer,
UiConfig as RawUIConfig,
} from './UiConfig';
import { UI_CONFIG } from '../../constants/api';

afterEach(() => {
fetchMock.restore();
vi.resetModules();
});

beforeEach(() => {
// We fetch the resource on module initialization for performance, but want to do that in every
// test.
});

const UiConfig = () => {
return (
const renderUiConfig = async () => {
const { UiConfigConsumer, UiConfig } = await import('./UiConfig');
render(
<Suspense fallback="Suspended">
<RawUIConfig>
<UiConfig>
<UiConfigConsumer>
{(value) => <div>{JSON.stringify(value)}</div>}
</UiConfigConsumer>
</RawUIConfig>
</Suspense>
</UiConfig>
</Suspense>,
);
};

describe('<UiConfig />', () => {
it('fetches config and suspends', async () => {
const configPromise = new Promise(() => undefined);
const { UI_CONFIG } = await import('../../constants/api');
fetchMock.once(UI_CONFIG, configPromise, { overwriteRoutes: true });

render(<UiConfig />);
await renderUiConfig();
expect(screen.getAllByText('Suspended')).length(1);

fetchMock.called(UI_CONFIG);
});

it('provides config when resolved', async () => {
it('provides config when resolved', (context) => async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

since this file was renamed in the last commit, none of the tests ran. I was able to get the former to run and pass, but it is a bit hacky as I really have no idea what I'm doing.

@anuraaga this is on a shared branch.. I think your old test was trying to show that the UI can work while the config.json request hasn't completed yet? I tried for over an hour to try to get this to work, so am not bothering you randomly.. could really use a hand (even you can paste) if you can help get this working.

Copy link
Member Author

Choose a reason for hiding this comment

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

#3724 for follow-up

const config = { defaultLookback: 100 };
const { defaultConfig } = await import('./UiConfig');
Object.keys(defaultConfig).forEach((key) => {
config[key] = config[key] || defaultConfig[key];
});

const { UI_CONFIG } = await import('../../constants/api');
fetchMock.once(UI_CONFIG, config, { overwriteRoutes: true });

render(<UiConfig />);
expect(screen.getAllByText('Suspended').length).toBe(1);
// TODO: adrian needs help, as this broke when porting to vitest
context.skip();

const { rerender } = await renderUiConfig();
expect(screen.getAllByText('Suspended')).length(1);

/*// We need to get off the processing loop to allow the promise to complete and resolve the
// We need to get off the processing loop to allow the promise to complete and resolve the
// config.
await new Promise((resolve) => setTimeout(resolve, 1));

rerender(<UiConfig />);
expect(screen.getByText(JSON.stringify(config))).toBeInTheDocument();*/
rerender(); // was rerender(<UiConfig />);
expect(screen.getByText(JSON.stringify(config))).toBeInTheDocument();

fetchMock.called(UI_CONFIG);
});
Expand Down
39 changes: 39 additions & 0 deletions zipkin-lens/src/constants/api.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

/* eslint-disable global-require */

import { afterEach, it, describe, expect, vi } from 'vitest';
Copy link
Member Author

Choose a reason for hiding this comment

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

this file was deleted in the last commit. I resurrected it.


// To test BASE_PATH we can't import it above, as it would return a constant
// value for all tests. Instead, we use `await import` later.
describe('BASE_PATH', () => {
afterEach(() => {
vi.resetModules();
});

it('defaults to /zipkin with no base tag', async () => {
const { BASE_PATH } = await import('./api');
Copy link
Member Author

Choose a reason for hiding this comment

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

ps I barely understand what I was doing here, but by behavior, I notice you have to defer anything you want reverted by resetModules. This was the only way I could monkey it to work, and if is not the correct way, please suggest an alternative!

expect(BASE_PATH).toEqual('/zipkin');
});

it('is set to base tag when present', async () => {
const base = document.createElement('base');
base.setAttribute('href', '/coolzipkin/');
document.head.append(base);

const { BASE_PATH } = await import('./api');
expect(BASE_PATH).toEqual('/coolzipkin');
});
});
13 changes: 11 additions & 2 deletions zipkin-lens/src/constants/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,17 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
export const BASE_PATH = import.meta.env.BASE_PATH;
export const ZIPKIN_API = `${import.meta.env.API_BASE}/api/v2`;
const extractBasePath = () => {
const base = document.getElementsByTagName('base');
if (base.length === 0) {
return '/zipkin';
}
return base[0]?.getAttribute('href')?.replace(/\/+$/, '');
};

export const BASE_PATH = extractBasePath();

export const ZIPKIN_API = `${BASE_PATH}/api/v2`;
export const UI_CONFIG = `${BASE_PATH}/config.json`;
export const SERVICES = `${ZIPKIN_API}/services`;
export const REMOTE_SERVICES = `${ZIPKIN_API}/remoteServices`;
Expand Down
27 changes: 0 additions & 27 deletions zipkin-lens/src/setupProxy.js

This file was deleted.

3 changes: 1 addition & 2 deletions zipkin-lens/src/setupTests.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2023 The OpenZipkin Authors
* Copyright 2015-2024 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -11,7 +11,6 @@
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import '@testing-library/jest-dom';
import fetchMock from 'fetch-mock';

Expand Down
Loading
Loading